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 and run clang-format #1368

Merged
merged 4 commits into from
Feb 6, 2024
Merged

Conversation

KrisThielemans
Copy link
Collaborator

@KrisThielemans KrisThielemans commented Feb 5, 2024

Finally enact #98

update: use one-per-line for parameters/arguments if they don't fit on one line. @danieldeidda @markus-jehl @casperdcl @dvolgyes any final comments on this file?

I will also push changes here, after one more PR is merged. This will therefore be rebased, commits dropped etc until happy.

@casperdcl
Copy link
Collaborator

casperdcl commented Feb 5, 2024

apart from the (very minor) pointless indentation of some values in this file, lgtm

@KrisThielemans
Copy link
Collaborator Author

I ran

pre-commit install
pre-commit run --all-files
git commit -a --author="stir_maintenance <[email protected]>"

@KrisThielemans
Copy link
Collaborator Author

This looks largely ok. I'm surprised by the following change:

- General_Reconstruction::
- General_Reconstruction()
- {
-    this->set_defaults();
- }
+ General_Reconstruction::General_Reconstruction() { this->set_defaults(); }

I thought I was aiming for

General_Reconstruction::General_Reconstruction()
{
  this->set_defaults();
}

or something similar. Anyone knows what setting this is?

@KrisThielemans KrisThielemans added this to the v6.0 milestone Feb 5, 2024
use one-per-line for parameters/arguments if they don't fit on one line

set AllowShortFunctionsOnASingleLine to InlineOnly, although it
doesn't seem to quite work properly, i.e. it no longer puts short functions
in the class on one line, but it does "spread" out-of-class definitions.
@KrisThielemans
Copy link
Collaborator Author

@casperdcl how does this work with git-fame. We run it to ignore white-space changes, but I guess this is per-line.

git fame -wMC --excl '\.(eps|root|ahv|hv|v|hs|s|scan|l|hdr|rtf|gz|if|pdf|safir|options|png|cls|sty)$|external_helpers|crystal_map|collimator.*txt|Doxyfile.in|LICENSE.txt|LICENSES' \
| tee git-fame-output.txt

Are all these non-trivial lines going to be attributed to stir-maintainer? If so, not much we can do about it I guess.

@KrisThielemans
Copy link
Collaborator Author

I couldn't get @ashgillman's suggested process #724 (comment) and #724 (comment) to work (I didn't rebase though). In the end, it looks like the following will have to be our recommendation for an existing branch. (replacing clangformat with origin/master after the merge of course).

git merge  clangformat -Xignore-all-space
# remove conflicts
pre-commit run --all-files

@casperdcl is this ok?

@KrisThielemans
Copy link
Collaborator Author

Ready so I'll merge. @casperdcl still interested in the questions above.

@KrisThielemans KrisThielemans merged commit 0dc76e5 into UCL:master Feb 6, 2024
7 of 8 checks passed
@KrisThielemans KrisThielemans deleted the clangformat branch February 6, 2024 15:11
@KrisThielemans
Copy link
Collaborator Author

@casperdcl git fame -wMC... currently returns

| Author                 |    loc |   coms |   fils |  distribution   |
|:-----------------------|-------:|-------:|-------:|:----------------|
| Kris Thielemans        | 186580 |   6150 |   1646 | 63.4/69.5/25.2  |
| stir_maintenance       |  33628 |      3 |   1123 | 11.4/ 0.0/17.2  |
| Nikos Efthimiou        |  11496 |    367 |    281 | 3.9/ 4.1/ 4.3   |

This is a bit sad. I've created casperdcl/git-fame#94...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

fix and enforce convention on white-spaces, code-indentation and other coding style
3 participants