clang-format improvements by beinhaerter · Pull Request #1251 · microsoft/GSL

beinhaerter · 2026-05-28T12:37:00Z

Add a clang-format linter check to the PR pipeline
Add a Windows bat file and a Linux sh script to apply formatting to all sources files, and on Linux also to all .sh shell scripts. Add a linter check to the pipeline for the .sh shell scripts (calling shfmt and shellcheck).
Run the formatting script(s) to apply clang-format and fix the clang-format linter errors
Remove CommentPragmas from .clang-format
Add WhitespaceSensitiveMacros: [GSL_SUPPRESS] to .clang-format
Remove all // clang-format off and // NO-FORMAT as they are not needed any more
Remove a commented out GSL_SUPPRESS

- 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 · 2026-05-28T14:23:58Z

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

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.

beinhaerter · 2026-05-29T09:39:23Z

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.

Co-authored-by: Carson Radtke <nosrac925@gmail.com>

…"GSL_SUPPRESS(bounds .1)"

to prevent formatting of include/CMakeLists.txt

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 · 2026-05-29T16:01:55Z

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 · 2026-05-29T16:02:56Z

+      # Install the exact clang-format binary
+      - name: Install clang-format
+        run: |
+          sudo apt install clang-format-${{ env.CLANG_VERSION }}


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

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?

Shall I add those files to make formatting easier?

That sounds good to me. Can you add it to a <repo-root>/scripts folder?

Alternatively, we could add it as a copilot skill so [at]Copilot can make the changes automatically?

This way we don't have to worry about what clang-format version is installed on the user's machine when they run the script. Copilot will automatically have whatever we specify in https://github.com/microsoft/GSL/blob/main/.github/workflows/copilot-setup-steps.yml.

Either way is fine by me.

Regarding the copilot-setup-steps.yml I have no idea how that all works together, what the best place is to do changes. I either need your guidance or you do your refinements after this PR is merged. Whatever you think is better.

carsonRadtke

Thanks for the changes! LGTM

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

beinhaerter · 2026-05-29T19:55:15Z

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()])
 {

carsonRadtke · 2026-06-01T15:57:26Z

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?

I don't really have a preference on what "looks better", but I do think that having WhitespaceSensitiveMacros: [GSL_SUPPRESS] is correct and should be kept. My vote is "apply the patch".

for linux with linter (shfmt and shellcheck)

beinhaerter mentioned this pull request May 28, 2026

remove space in GSL_SUPPRESS #1248

Merged

beinhaerter force-pushed the clang-format branch from d0cba15 to 060a2c8 Compare May 28, 2026 12:42

beinhaerter changed the title ~~clang-format~~ clang-format improvements May 28, 2026

beinhaerter force-pushed the clang-format branch 2 times, most recently from 1bd5793 to 5b3e9be Compare May 28, 2026 13:25

beinhaerter marked this pull request as ready for review May 28, 2026 13:25

beinhaerter force-pushed the clang-format branch from 5b3e9be to ca0fed2 Compare May 28, 2026 13:29

clang-format 20

26a2b6a

pipeline fail

e8dcfbe

carsonRadtke requested changes May 28, 2026

View reviewed changes

Comment thread tests/utils_tests.cpp

Comment thread tests/span_tests.cpp Outdated

Comment thread .github/workflows/clang-format.yml Outdated

beinhaerter and others added 3 commits May 29, 2026 11:49

Update .github/workflows/clang-format.yml

1b433f0

Co-authored-by: Carson Radtke <nosrac925@gmail.com>

output used clang-format version

cef28cf

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

27ec152

…"GSL_SUPPRESS(bounds .1)"

beinhaerter force-pushed the clang-format branch from 82ecfc3 to 27ec152 Compare May 29, 2026 10:32

Werner Henze added 2 commits May 29, 2026 14:29

only include/gsl, not include

8e6f2a0

to prevent formatting of include/CMakeLists.txt

carsonRadtke reviewed May 29, 2026

View reviewed changes

carsonRadtke approved these changes May 29, 2026

View reviewed changes

change "#if 0" back to original version with comments only

f49ce47

Werner Henze added 2 commits June 1, 2026 22:38

formatting scripts for windows and linux

32581bf

for linux with linter (shfmt and shellcheck)

add WhitespaceSensitiveMacros: [GSL_SUPPRESS]

a0ccc99

Conversation

beinhaerter commented May 28, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

beinhaerter commented May 28, 2026

Uh oh!

carsonRadtke left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

beinhaerter commented May 29, 2026

Uh oh!

carsonRadtke commented May 29, 2026

Uh oh!

carsonRadtke May 29, 2026

Choose a reason for hiding this comment

Uh oh!

beinhaerter May 29, 2026

Choose a reason for hiding this comment

Uh oh!

carsonRadtke Jun 1, 2026

Choose a reason for hiding this comment

Uh oh!

carsonRadtke Jun 1, 2026

Choose a reason for hiding this comment

Uh oh!

beinhaerter Jun 1, 2026

Choose a reason for hiding this comment

Uh oh!

carsonRadtke left a comment

Choose a reason for hiding this comment

Uh oh!

beinhaerter commented May 29, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

carsonRadtke commented Jun 1, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

beinhaerter commented May 28, 2026 •

edited

Loading

beinhaerter commented May 29, 2026 •

edited

Loading