Skip to content

update and run clang-format#1368

Merged
KrisThielemans merged 4 commits into
UCL:masterfrom
KrisThielemans:clangformat
Feb 6, 2024
Merged

update and run clang-format#1368
KrisThielemans merged 4 commits into
UCL:masterfrom
KrisThielemans:clangformat

Conversation

@KrisThielemans

@KrisThielemans KrisThielemans commented Feb 5, 2024

Copy link
Copy Markdown
Collaborator

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

casperdcl commented Feb 5, 2024

Copy link
Copy Markdown
Collaborator

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

@KrisThielemans

Copy link
Copy Markdown
Collaborator Author

I ran

pre-commit install
pre-commit run --all-files
git commit -a --author="stir_maintenance <noreply@github.com>"

@KrisThielemans

Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
@KrisThielemans KrisThielemans deleted the clangformat branch February 6, 2024 15:11
@KrisThielemans

Copy link
Copy Markdown
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...

@casperdcl

Copy link
Copy Markdown
Collaborator

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