Skip to content

clang-format improvements#1251

Open
beinhaerter wants to merge 9 commits into
microsoft:mainfrom
beinhaerter:clang-format
Open

clang-format improvements#1251
beinhaerter wants to merge 9 commits into
microsoft:mainfrom
beinhaerter:clang-format

Conversation

@beinhaerter
Copy link
Copy Markdown
Contributor

@beinhaerter beinhaerter commented May 28, 2026 •
edited
Loading

  • Add a clang-format linter check to the PR pipeline
  • Apply clang-format to files where the linter initially failed
  • Remove CommentPragmas from .clang-format
  • Remove all // clang-format off and // NO-FORMAT as they are not needed
  • Remove a commented out GSL_SUPPRESS

@beinhaerter beinhaerter mentioned this pull request May 28, 2026
Merged
@beinhaerter beinhaerter changed the title clang-format clang-format improvements May 28, 2026
@beinhaerter beinhaerter force-pushed the clang-format branch 2 times, most recently from 1bd5793 to 5b3e9be Compare May 28, 2026 13:25
@beinhaerter beinhaerter marked this pull request as ready for review May 28, 2026 13:25
clang-format improvements
ca0fed2 
- Add a clang-format linter check to the PR pipeline
- Apply clang-format to files where the linter initially failed
- Remove `CommentPragmas` from `.clang-format`
- Remove all `// clang-format off` and `// NO-FORMAT` as they are not needed
- Remove a commented out `GSL_SUPPRESS`
@beinhaerter
Copy link
Copy Markdown
Contributor Author

beinhaerter commented May 28, 2026

I am not github pipeline expert. The clang-format.yml file is AI generated. I have no idea if it is OK or not.

carsonRadtke
carsonRadtke requested changes May 28, 2026
View reviewed changes
Copy link
Copy Markdown
Member

@carsonRadtke carsonRadtke left a comment

Choose a reason for hiding this comment

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

Thanks for making these changes! The only blocker is the third-party actions dependency - if you wouldn't mind changing that to a shell command, that'd be appreciated.

Comment thread tests/utils_tests.cpp
Comment thread tests/span_tests.cpp Outdated
Comment thread .github/workflows/clang-format.yml Outdated
@beinhaerter
Copy link
Copy Markdown
Contributor Author

beinhaerter commented May 29, 2026

Thanks for making these changes! The only blocker is the third-party actions dependency - if you wouldn't mind changing that to a shell command, that'd be appreciated.

OK, I'll give it a try.

Werner Henze added 2 commits May 29, 2026 14:29
only include/gsl, not include
8e6f2a0 
to prevent formatting of include/CMakeLists.txt
apply clang-format[-20]
dc788cf 
In a VS2026 developer command prompt I ran `clang-format -i include\gsl\* --assume-filename x.cpp`. This was necessary because VS GUI does not format files without an extension :(. Please note that `--assume-filename` is necessary here, otherwise the files will not be formatted. Surprisingly the behaviour for the formatter differs from the behaviour of `lang-format(-20) --dry-run --Werror include/gsl/*` where clang-format recognizes that the files is C++.
@carsonRadtke
Copy link
Copy Markdown
Member

carsonRadtke commented May 29, 2026

RE: installed version is 18 which replaces "GSL_SUPPRESS(bounds.1)" with "GSL_SUPPRESS(bounds .1)"

Consider adding WhitespaceSensitiveMacros: [GSL_SUPPRESS] to .clang-format so this doesn't happen?

carsonRadtke
carsonRadtke reviewed May 29, 2026
View reviewed changes
Comment thread .github/workflows/clang-format.yml
Comment on lines +24 to +27
# Install the exact clang-format binary
- name: Install clang-format
run: |
sudo apt install clang-format-${{ env.CLANG_VERSION }}
Copy link
Copy Markdown
Member

@carsonRadtke carsonRadtke May 29, 2026

Choose a reason for hiding this comment

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

I'd rather use the pre-installed version, but I'm not going to block the PR on it.

Copy link
Copy Markdown
Contributor Author

@beinhaerter beinhaerter May 29, 2026

Choose a reason for hiding this comment

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

The pre-installed version needs the WhitespaceSensitiveMacros change, clang-format-20 does not.

Regarding the burden on the authors: I think it might be a good idea to provide an apply-clang-format.bat and an apply-clang-format.sh. Of course the bat file would only work in the developer command prompt and could only use the clang-format version shipped with Visual Studio. What do you think about it? Shall I add those files to make formatting easier?

carsonRadtke
carsonRadtke approved these changes May 29, 2026
View reviewed changes
Copy link
Copy Markdown
Member

@carsonRadtke carsonRadtke left a comment

Choose a reason for hiding this comment

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

Thanks for the changes! LGTM

I'll merge it on Monday in case you want to address my non-blocking comments.

@beinhaerter
Copy link
Copy Markdown
Contributor Author

beinhaerter commented May 29, 2026 •
edited
Loading

RE: installed version is 18 which replaces "GSL_SUPPRESS(bounds.1)" with "GSL_SUPPRESS(bounds .1)"

Consider adding WhitespaceSensitiveMacros: [GSL_SUPPRESS] to .clang-format so this doesn't happen?

Here is a patch for .clang-format together with the changes it does to the files. I understand that it fixes the whitespace-in-suppress-attributes-problem, but I am not sure if this looks better. Which version do you prefer? Shall I apply the patch or leave it as it is?

For example:

 template <class Cont>
-GSL_SUPPRESS(bounds.4)
-GSL_SUPPRESS(bounds.2) constexpr auto at(Cont& cont, const index i) -> decltype(cont[cont.size()])
+GSL_SUPPRESS(bounds.4) GSL_SUPPRESS(bounds.2) constexpr auto at(Cont& cont, const index i)
+    -> decltype(cont[cont.size()])
 {

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@carsonRadtke carsonRadtke carsonRadtke approved these changes

At least 1 approving review is required to merge this pull request.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@beinhaerter @carsonRadtke