Repository navigation
remove space in GSL_SUPPRESS - #1248
Carson Radtke (carsonRadtke) merged 1 commit into
Conversation
As VS2026 wants a string literal in `[[gsl::suppress(...)]]` it is a difference, if you `GSL_SUPPRESS(a.1)` without space or `GSL_SUPPRESS(a .1)` with space. The version with the space does not work, so this commit removes the spaces.
| { | ||
| // clang-format off | ||
| GSL_SUPPRESS(f .4) // NO-FORMAT: attribute | ||
| GSL_SUPPRESS(f.4) // NO-FORMAT: attribute |
There was a problem hiding this comment.
Most lines with GSL_SUPPRESS have a trailing // NO-FORMAT: attribute. I have left it here, but when we remove the NO-FORMAT, the VS formatting already fixes the f .4 to f.4.
I not only don't see a reason to write NO-FORMAT, I also think it makes things worse. I vote to remove all NO-FORMAT on GSL_SUPPRESS and revisit the necessity of CommentPragmas: '^ NO-FORMAT:' in .clang-format. If you agree, I can do that in another PR.
There was a problem hiding this comment.
VS formatting already fixes the f .4 to f.4.
Does this happen with older toolsets? Specifically, when GSL_SUPPRESS(abc.123) expands to [[gsl::suppress(abc.123)]] instead of [[gsl::suppress("abc.123)]]?
revisit the necessity of CommentPragmas: '^ NO-FORMAT:' in .clang-format
I'm not running clang-format on every change. Seems like we are not compliant with what clang-format wants either:
PS C:\Users\carsonradtke\src\GSL\include\gsl> clang-format -i *
is a directory
is a directory
PS C:\Users\carsonradtke\src\GSL\include\gsl> git diff --stat
include/gsl/byte | 32 +++++++++-------
include/gsl/gsl | 14 +++----
include/gsl/narrow | 29 +++++++--------
include/gsl/pointers | 101 ++++++++++++++++++++++++++++++---------------------
include/gsl/span | 17 ++++++---
include/gsl/util | 28 +++++++++-----
include/gsl/zstring | 2 +-
7 files changed, 130 insertions(+), 93 deletions(-)
PS C:\Users\carsonradtke\src\GSL\include\gsl> If you agree, I can do that in another PR.
Sounds good to me. In addition to the CommentPragmas change, maybe also add a GitHub action that requires $ clang-format -i --dry-run include/gsl/* tests/*.{cpp,h} to exit successfully? This adds friction to people submitting PRs, but at least we have an agreed upon style. Your choice.
There was a problem hiding this comment.
Does this happen with older toolsets?
I'll validate this afternoon and if all is good, I'll approve and merge.
There was a problem hiding this comment.
Er - on older toolsets this wouldn't matter because abc.123 would be parsed the same way as abc. 123. However, I'm surprised that clang-format makes this change when the macro is expanded with the argument as a string. This type of refactor changes the meaning of the program. In the case of GSL_SUPPRESS the reactor is safe, but not in general. Anyway, merging now.
Edit: typo
Carson Radtke (carsonRadtke)
left a comment
Carson Radtke (carsonRadtke)
left a comment
There was a problem hiding this comment.
LGTM and feel free to remove the NO-FORMAT stuff in a future PR.
f3c5967
into
microsoft:main
cpp-gsl 5.0.0 Created-by: HarmonybrewBot Commit-by: HarmonybrewBot Merged-by: HarmonybrewBot Description: Created by `brew bump` --- Created with `brew bump-formula-pr`.<details> <summary>release notes</summary> <pre>## What's Changed * Update return by value threshold in gsl::not_null by @ozaktash in microsoft/GSL#1205 * copilot: Provide more project context for the Copilot coding agent by @carsonRadtke in microsoft/GSL#1207 * Document differences between hardened std::span and gsl::span by @carsonRadtke with @Copilot in microsoft/GSL#1206 * Add constexpr to not_null comparison operators by @carsonRadtke with @Copilot in microsoft/GSL#1208 * fix: all pipeline failures by @carsonRadtke in microsoft/GSL#1221 * export proper syntax for GSL_SUPPRESS for new VS by @carsonRadtke in microsoft/GSL#1213 * Revert unintended change to GSL_SUPPRESS by @beinhaerter in microsoft/GSL#1226 * use cmake presets instead of cmake settings by @carsonRadtke in microsoft/GSL#1219 * Fix std::hash compilation error for gsl::not_null<std::shared_ptr<T>> by @carsonRadtke with @Copilot in microsoft/GSL#1210 * infra: individual test executables by @carsonRadtke in microsoft/GSL#1212 * doc: Update URL in pointers to F.16 C++ Core Guidelines by @maflcko in microsoft/GSL#1232 * doc: Fix to use lower-case anchors in links to C++ Core Guidelines by @maflcko in microsoft/GSL#1234 * gsl::not_null: C.85: Make swap noexcept by @maflcko in microsoft/GSL#1235 * gsl::not_null: C.89: Make a hash noexcept by @maflcko in microsoft/GSL#1236 * ci: Bump to actions/checkout@v6 by @maflcko in microsoft/GSL#1237 * ci: Use microsoft/setup-msbuild@v3 by @maflcko in microsoft/GSL#1240 * narrow_cast instead of static_cast by @beinhaerter in microsoft/GSL#1250 * ci: Bump actions/setup-java@v5 by @maflcko in microsoft/GSL#1249 * remove space in GSL_SUPPRESS by @beinhaerter in microsoft/GSL#1248 * clang-format improvements by @beinhaerter in microsoft/GSL#1251 * feat: implementation of gsl::dyn_array by @carsonRadtke in microsoft/GSL#1228 * ci: fix failing actions by @carsonRadtke in microsoft/GSL#1255 * docs: fix false statements in gsl::dyn_array docs by @carsonRadtke in microsoft/GSL#1254 * dyn_array: fix include tree by @carsonRadtke in microsoft/GSL#1261 * Bump GSL to v5.0.0 by @carsonRadtke in microsoft/GSL#1262 * docs: fix http:// to https:// in CppCoreGuidelines links by @Rajkaran-122 in microsoft/GSL#1267 * Fix floating-to-integral narrow undefined behavior by @carsonRadtke in microsoft/GSL#1266 * Ensure iterators convert to const iterators by @carsonRadtke in microsoft/GSL#1264 * Prevent external construction of checked iterators by @carsonRadtke in microsoft/GSL#1263 ## New Contributors * @ozaktash made their first contribution in microsoft/GSL#1205 * @maflcko made their first contribution in microsoft/GSL#1232 * @Rajkaran-122 made their first contribution in microsoft/GSL#1267 **Full Changelog**: https://github.com/microsoft/GSL/compare/v4.2.2...v5.0.0</pre> <p>View the full release notes at <a href="https://github.com/microsoft/GSL/releases/tag/v5.0.0">https://github.com/microsoft/GSL/releases/tag/v5.0.0</a>.</p> </details> <hr> See merge request: Harmonybrew/homebrew-core!17783
As VS2026 wants a string literal in
[[gsl::suppress(...)]]it is a difference, if youGSL_SUPPRESS(a.1)without space orGSL_SUPPRESS(a .1)with space. The version with the space does not work, so this commit removes the spaces.