Skip to content

Conversation

cacharle
Copy link
Contributor

The formatter hasn't been run in a long time so I formatted all the files (with the exact same formatting config as before) and added a check in CI/CD.

I feel like having a pre-commit config to run clang-format before commiting might be handy aswell

@coveralls
Copy link

coveralls commented Oct 17, 2025

Pull Request Test Coverage Report for Build 18599605414

Details

  • 44 of 44 (100.0%) changed or added relevant lines in 2 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.02%) to 88.402%

Totals Coverage Status
Change from base Build 17909983277: 0.02%
Covered Lines: 907
Relevant Lines: 1026

💛 - Coveralls

Comment on lines +33 to +41
// To avoid the `(std::min)` workaround for when a min macro is defined
#ifdef min
#define WINDOWS_MIN min
#undef min
#endif
#ifdef max
#define WINDOWS_MAX max
#undef max
#endif
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was initially to fix the formatting being different between clang-format 14 and 20 for the (std::min)(x, y) trick.

I ended up fixing it differently but this "guard" macros seems a bit cleaner than having to remember the (std::min) workaround. Let me know if I should keep it

(Still have to test if it works on Windows tho)

Comment on lines 135 to 140
- name: format
if: ${{ startsWith(matrix.os, 'ubuntu-24') }}
run: |
clang-format --version
git ls-files | grep -E '\.(cpp|hpp)' | xargs clang-format --dry-run --Werror
sudo apt install -y clang-format-20
clang-format-20 --version
git ls-files | grep -E '\.(cpp|hpp)' | xargs clang-format-20 --dry-run --Werror
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is running clang-format 20 only on Ubuntu okay?

Not sure how to solve the fact that people might have different versions installed..

@cacharle cacharle marked this pull request as ready for review October 17, 2025 16:59
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.

2 participants