-
Notifications
You must be signed in to change notification settings - Fork 54
clang-format #1032
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
clang-format #1032
Conversation
4bbf790 to
79dafdd
Compare
a789bf8 to
5f89c09
Compare
c00dd15 to
c239eea
Compare
|
I think we can use pre-commit.ci to run this on PRs and automatically let it push a "fix" on a PR for style: Seen in PRs to |
619e346 to
b7ebdef
Compare
f2cec0f to
7229cbf
Compare
|
I might have gotten this to work |
.pre-commit-config.yaml
Outdated
| repos: | ||
| - repo: https://github.com/franzpoeschel/openPMD-api-pre-commit-hooks | ||
| rev: aa38b2ad4fae42db05c19de211f43df6a7981bc5 | ||
| - repo: local |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could use https://github.com/pre-commit/mirrors-clang-format for this :)
Alternatively, if we want to use our own scripts and env, let's move them in a sub-directory in .github/workflows/clang-format/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like the current solution actually because it gives us a script that users can also call "bare-metal" if they want, without having to set up pre-commit. Also, it's a 5-line script that saves us from relying on an external dependency.
I can move it to the workflows thing, sure.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Jup, I think that's a good point and it integrates well.
c92c6b0 to
79ca528
Compare
|
Would you think writing a small Python script that helps us migrate old pull requests past a reformatting of the whole code base is worth it? I think it should be possible. EDIT: I needed this somewhere else too, so I quickly hacked something up https://github.com/franzpoeschel/reformat-rebase/blob/master/reformat-rebase.py. Needs some documentation to get usable, but we don't need to wait on my PRs to merge the clang-format thing. I can also help rebasing other PRs past the reformat if anyone wants. |
2322590 to
92243da
Compare
92243da to
c7dbf24
Compare
|
I think I've found a solution/workaround for the
|
1) Change OPENPMD_private macros so clang-format understands them 2) Put HDF5 lint back to the right place 3) Fix includes: sorting the includes via clang-format uncovered an include error.
852e1ce to
0404103
Compare
|
Merged to I will need to apply this in the right order to the 0.14.5 branch, otherwise back-porting newer fixes like #1197 becomes very tricky :) |
Add a
.clang-formatfile to be used for the whole source tree. Close #1024.TODO
OPENPMD_private:Update:
Using clang-format-12 now Switched to clang-format-10 for now since I couldn't get version 12 running in the CI. Version 12 has some seriously improved handling of lambdas, so we might want to try that again.With pre-commit, we're now using clang-format-12 installed from Conda
--author="Tools <[email protected]>for now. Any other suggestion? Axel: ok, but in a separate commit/PR, because we squash PRs. Will cherry-pick on master and let the tool fix it.My personal preferences for formatting:
I don't care too much about tabs vs. spaces, placement of brackets on new lines or at the end of lines.
Guide for rebasing old PRs: ComputationalRadiationPhysics/picongpu#3464
Example for current error message when a wrong formatting is detected: https://github.com/openPMD/openPMD-api/runs/3016610168
Some discussion points:
I have currently specified
NamespaceIndentation=Inner. This unfortunately also indents something like this:(Adding
CompactNamespaces=truedoes not help here since that one is apparently ignored?)EDIT: This situation can be fixed upon
C++17by simply writingnamespace openPMD::detail {.Currently, we often use
if( boolean ). Maybe turn that intoif (boolean)? These spaces in parens are cumbersome to write and they take up space in nested calls.EDIT: Done this now.