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

Run clang-format on the repo, add a github action to check formatting #577

Merged
merged 7 commits into from
Feb 3, 2024

Conversation

SLieve
Copy link
Contributor

@SLieve SLieve commented Jan 28, 2024

  • Added a few clang-format directives to maintain ordering of includes:
    <angle-bracket includes> at the top
    
    <roaring/anything> below that
    
    "quoted includes" below that
    
  • Formatted the repo with tools/clang-format.sh
  • Marked the formatting commit as to-be-ignored by git blame, this should work with github too.
  • Reordered some includes where there was still breakage, added comments to prevent futher reordering.
  • Added a github action to check format against clang-format.

@SLieve SLieve marked this pull request as ready for review January 28, 2024 19:42
@SLieve SLieve requested a review from lemire January 28, 2024 19:42
@lemire
Copy link
Member

lemire commented Jan 29, 2024

Can you add instructions related to clang-cl under: .github/PULL_REQUEST_TEMPLATE.md and maybe somewhere as part of the README.md, or in an CONTRIBUTING.md file.

I don't object to your change, but I don't want to have to explain to people contributed code that they need to use clang-cl. This needs to be documented. Otherwise it creates a lot of bureaucratic exchanges.

Be mindful that not everyone uses clang-cl, so that's not entirely contributor friendly. (I personally do not object, to be clear.)

@SLieve
Copy link
Contributor Author

SLieve commented Jan 29, 2024

Done. Note that the requirements are actually even a bit more strict: run a specific version of clang-format (currently stable version 17). This avoids inconsistent formatting. If you have any suggestion on making this smoother, I'm happy to take them onboard.

@lemire
Copy link
Member

lemire commented Jan 29, 2024

@Dr-Emann Can you review?

@Dr-Emann
Copy link
Member

Might be worth a dockerfile with clang format installed, and a script that tries clang-format (checking the version), clang-format-17 and if neither work, using a docker container with the code mounted as a volume, to make it easier to format locally.

@lemire
Copy link
Member

lemire commented Jan 30, 2024

@Dr-Emann I coded something up with Docker: #580

Requiring users to have docker is a bit bad, but at least it will be consistent once they have it. Chasing the right clang-format version is terrible.

@SLieve
Copy link
Contributor Author

SLieve commented Jan 30, 2024

Nice, yes using a bundled docker container sounds much nicer.

Copy link
Member

@lemire lemire left a comment

Choose a reason for hiding this comment

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

We should be able to merge this.

Copy link
Member

@Dr-Emann Dr-Emann left a comment

Choose a reason for hiding this comment

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

Personally prefer a longer line length, but happy to have automated formatting, like they say about gofmt:

Gofmt's style is no one's favorite, yet gofmt is everyone's favorite.

README.md Outdated Show resolved Hide resolved
This allows anyone to ignore the commit with:
   git blame --ignore-revs-file .git-blame-ignore-revs

Or to always use the ignore-revs file:
    git config blame.ignoreRevsFile .git-blame-ignore-revs
The github action expects a .clang-format-ignore file in order to exclude files,
so I've renamed it. It also expects the entries to match the full path as far as
I can tell, so I've switched them to be regexes rather than plain filenames.
@SLieve SLieve merged commit 37c0388 into RoaringBitmap:master Feb 3, 2024
20 checks passed
@SLieve SLieve deleted the format branch February 3, 2024 12:40
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.

3 participants