Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update clang-format to version 9 #2235

Merged
merged 15 commits into from
Jan 20, 2022
Merged

Update clang-format to version 9 #2235

merged 15 commits into from
Jan 20, 2022

Conversation

jougs
Copy link
Contributor

@jougs jougs commented Dec 7, 2021

The title has it all.

  • 7b224a5 updates the static code analysis scripts and documentation
  • 2364a40 updates all files to the new formatting

@jougs jougs self-assigned this Dec 7, 2021
@jougs jougs added I: Internal API Changes were introduced in basic internal workings of the simulator that developers need to know S: Normal Handle this with default priority T: Maintenance Work to keep up the quality of the code and documentation. labels Dec 7, 2021
@jougs jougs added this to PRs in progress in Kernel via automation Dec 7, 2021
@jougs
Copy link
Contributor Author

jougs commented Dec 8, 2021

I think you can review now. Most of the changes are auto-generated, so you might want to restrict yourself to the manual changes in documentation and scripts pointed out in the description.

@heplesser
Copy link
Contributor

Is there a specific reason for using clang format 9, given that the current version of clang is 13?

@jougs
Copy link
Contributor Author

jougs commented Dec 8, 2021

Yes, there is. 9 is the version that is available in both Ubuntu 20.04 (LTS), which we're using on the CI, and on the most recent version 21.10. If we're using a newer version, we'd have to use the LLVM Ubuntu archive on the CI, which is of course also possible, but would require additional package repositories for developers still using an older version of Ubuntu.

@heplesser
Copy link
Contributor

That sounds like a reasonable reason for using 9. Maybe add a comment to the script to explain the choice, also for future reference. Is there any hope that clang-format at some point will make all rules configurable so we could just say clang >= N?

Copy link
Contributor

@terhorstd terhorstd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's much more readable and improves code style in many many places. 🎉 🥳
There are some spots that might need a brief look though. Maybe settings can fix that, but deeper code rewriting should of course not go into this PR.
Thanks a lot for diving into this! 🐠 🐟 🐚 🤿

build_support/format_all_c_c++_files.sh Outdated Show resolved Hide resolved
nestkernel/connector_base.h Show resolved Hide resolved
nestkernel/node_manager.cpp Show resolved Hide resolved
nestkernel/random_manager.cpp Show resolved Hide resolved
nestkernel/source_table.cpp Outdated Show resolved Hide resolved
sli/name.cc Show resolved Hide resolved
sli/tokenstack.h Outdated Show resolved Hide resolved
nestkernel/target.h Show resolved Hide resolved
Kernel automation moved this from PRs in progress to PRs pending approval Dec 10, 2021
jougs and others added 2 commits December 13, 2021 22:19
Co-authored-by: Dennis Terhorst <terhorstd@users.noreply.github.com>
build_support/check_code_style.sh Outdated Show resolved Hide resolved
build_support/format_all_c_c++_files.sh Outdated Show resolved Hide resolved
nestkernel/connector_base.h Show resolved Hide resolved
nestkernel/node_manager.cpp Show resolved Hide resolved
nestkernel/random_manager.cpp Show resolved Hide resolved
nestkernel/target.h Show resolved Hide resolved
sli/name.cc Show resolved Hide resolved
Copy link
Contributor

@heplesser heplesser left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jougs Looks good in principle, but I ran into one technical problem, see below. I have only checked the "mechanics", not the effect on formatting.

build_support/check_code_style.sh Outdated Show resolved Hide resolved
Co-authored-by: Hans Ekkehard Plesser <hans.ekkehard.plesser@nmbu.no>
@jougs
Copy link
Contributor Author

jougs commented Jan 10, 2022

@heplesser: I've committed your suggestion. Does this resolve your issues?
@clinssen: Could you please also review? Thanks!

Copy link
Contributor

@heplesser heplesser left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jougs Thank, all works now under M1 and the check passes, testing all (or at least a lot of) files.

@jougs
Copy link
Contributor Author

jougs commented Jan 11, 2022

@terhorstd: are you happy with the latest changes? Thanks!

@jougs
Copy link
Contributor Author

jougs commented Jan 13, 2022

Maybe it would be good to write a mail to the user list about this one and warn that it could be wise to reformat the code in own branches using clang-format 9 before pulling master after this has been merged? Maybe the mail could include the following snippet?

wget --content-disposition "https://raw.githubusercontent.com/nest/nest-simulator/6b1f1cbb960eb476d672df1b6a0ec02fb1e803a2/build_support/format_all_c_c%2B%2B_files.sh"
bash format_all_c_c++_files.sh
rm format_all_c_c++_files.sh

@heplesser
Copy link
Contributor

Maybe it would be good to write a mail to the user list about this one and warn that it could be wise to reformat the code in own branches using clang-format 9 before pulling master after this has been merged? Maybe the mail could include the following snippet?

@jougs That sounds like a good idea. I'd suggest to send it out a day or so before you merge.

Copy link
Contributor

@terhorstd terhorstd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice! Thanks a lot! 👍

Kernel automation moved this from PRs pending approval to PRs approved Jan 19, 2022
Copy link
Contributor

@clinssen clinssen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cheers!

@terhorstd terhorstd merged commit d3dd85a into nest:master Jan 20, 2022
Kernel automation moved this from PRs approved to Done (PRs and issues) Jan 20, 2022
@jougs
Copy link
Contributor Author

jougs commented Jan 20, 2022

@terhorstd: can you please also send the mail to the NEST user list about how to get own branches updated before pulling upstream master? Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
I: Internal API Changes were introduced in basic internal workings of the simulator that developers need to know S: Normal Handle this with default priority T: Maintenance Work to keep up the quality of the code and documentation.
Projects
Kernel
  
Done
Development

Successfully merging this pull request may close these issues.

None yet

4 participants