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

clang-format #1032

Closed
wants to merge 4 commits into from
Closed

clang-format #1032

wants to merge 4 commits into from

Conversation

franzpoeschel
Copy link
Contributor

@franzpoeschel franzpoeschel commented Jul 7, 2021

Add a .clang-format file to be used for the whole source tree. Close #1024.

TODO

  • Discuss the formatting itself
  • Somehow fix indentation after OPENPMD_private:
  • Use a specific version of clang-format in the CI
    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
  • Bewitch Github to auto-push a commit fixing wrong formatting
  • This is a huge PR since it reformats the whole source tree. I've used --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.
  • Format the examples too (I forgot those)
  • Make sure before merging to set the author of the reformatting commit to "Tools"

My personal preferences for formatting:

  • A low line width (currently 80). I like to use wide monitors not for writing long lines, but for viewing multiple columns at the same time.
  • Careful use of horizontal screen estate. In doubt, don't indent if not necessary.
  • Indent only, avoid aligning. Reduces diff size in git and in manual editing.
  • No single-line if/while statements without braces. (No real benefit, but annoying when adding a second statement, e.g. for debugging)

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:

namespace openPMD
{
namespace detail
{
    // ADIOS2 does not natively support boolean values
    // Since we need them for attributes,
    // we represent booleans as unsigned chars
    using bool_representation = unsigned char;

(Adding CompactNamespaces=true does not help here since that one is apparently ignored?)

EDIT: This situation can be fixed upon C++17 by simply writing namespace openPMD::detail {.

Currently, we often use if( boolean ). Maybe turn that into if (boolean)? These spaces in parens are cumbersome to write and they take up space in nested calls.

EDIT: Done this now.

@franzpoeschel franzpoeschel force-pushed the clang-format branch 6 times, most recently from a789bf8 to 5f89c09 Compare July 7, 2021 13:42
@ax3l ax3l self-requested a review July 8, 2021 00:31
@ax3l ax3l self-assigned this Jul 8, 2021
@franzpoeschel franzpoeschel force-pushed the clang-format branch 2 times, most recently from c00dd15 to c239eea Compare July 8, 2021 07:25
@ax3l
Copy link
Member

ax3l commented Nov 4, 2021

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 cibuildwheel:
https://github.com/pypa/cibuildwheel/blob/main/.pre-commit-config.yaml

@franzpoeschel franzpoeschel force-pushed the clang-format branch 16 times, most recently from 619e346 to b7ebdef Compare November 8, 2021 15:57
@franzpoeschel franzpoeschel force-pushed the clang-format branch 2 times, most recently from f2cec0f to 7229cbf Compare November 8, 2021 16:03
@franzpoeschel
Copy link
Contributor Author

I might have gotten this to work
Needs clean-up though (and there are some rather long PRs I would like to see merged before this anyway)

@ax3l ax3l mentioned this pull request Nov 8, 2021
@@ -1,7 +1,11 @@
# See https://pre-commit.com for more information
# See https://pre-commit.com/hooks.html for more hooks
repos:
- repo: https://github.com/franzpoeschel/openPMD-api-pre-commit-hooks
rev: aa38b2ad4fae42db05c19de211f43df6a7981bc5
- repo: local
Copy link
Member

@ax3l ax3l Nov 8, 2021

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/

Copy link
Contributor Author

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.

Copy link
Member

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.

@franzpoeschel franzpoeschel force-pushed the clang-format branch 2 times, most recently from c92c6b0 to 79ca528 Compare November 9, 2021 10:20
@franzpoeschel
Copy link
Contributor Author

franzpoeschel commented Nov 9, 2021

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.

@franzpoeschel
Copy link
Contributor Author

I think I've found a solution/workaround for the OPENPMD_private and OPENPMD_protected macros:

  • Make the colon part of the macro, e.g. #define OPENPMD_private private:
  • Specify StatementMacros: ["OPENPMD_private", "OPENPMD_protected"] in .clang-format. This makes clang-format think that these are just normal statements
  • Use // clang-format off and // clang-format on around them. Inside these blocks, we can just put them to the right indentation manually, and outside these blocks they don't affect indentation since clang-format thinks that they're just normal statements.

franzpoeschel and others added 4 commits February 15, 2022 12:18
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.
@ax3l
Copy link
Member

ax3l commented Feb 18, 2022

Merged to dev! 🎉

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 :)

@ax3l ax3l closed this Feb 18, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add .clang-format file
2 participants