Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion README.md
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,7 @@ dyn_array | &#x26
move_owner | ☐ | A helper function that moves one `owner` to the other
[final_action](docs/headers.md#user-content-H-util-final_action) | ☑ | A RAII style class that invokes a functor on its destruction
[finally](docs/headers.md#user-content-H-util-finally) | ☑ | A helper function instantiating [final_action](docs/headers.md#user-content-H-util-final_action)
[GSL_SUPPRESS](docs/headers.md#user-content-H-assert-gsl_suppress) | ☑ | A macro that takes an argument and turns it into `[[gsl::suppress(x)]]` or `[[gsl::suppress("x")]]`
[GSL_SUPPRESS](docs/headers.md#user-content-H-assert-gsl_suppress) | ☑ | A macro that takes an argument and turns it into `[[gsl::suppress(x)]]` or `[[gsl::suppress("x")]]` depending on the compiler.
[[implicit]] | ☐ | A "marker" to put on single-argument constructors to explicitly make them non-explicit
[index](docs/headers.md#user-content-H-util-index) | ☑ | A type to use for all container and array indexing (currently an alias for `std::ptrdiff_t`)
[narrow](docs/headers.md#user-content-H-narrow-narrow) | ☑ | A checked version of `narrow_cast`; it can throw [narrowing_error](docs/headers.md#user-content-H-narrow-narrowing_error)
Expand Down
4 changes: 2 additions & 2 deletions docs/headers.md
Original file line number Diff line number Diff line change
Expand Up @@ -49,9 +49,9 @@ See [GSL.assert: Assertions](https://isocpp.github.io/CppCoreGuidelines/CppCoreG
This macro can be used to suppress a code analysis warning.

The core guidelines request tools that check for the rules to respect suppressing a rule by writing
`[[gsl::suppress(tag)]]` or `[[gsl::suppress(tag, justification: "message")]]`.
`[[gsl::suppress("tag")]]` or `[[gsl::suppress("tag", justification: "message")]]`.

Clang does not use exactly that syntax, but requires `tag` to be put in double quotes `[[gsl::suppress("tag")]]`.
Older versions of MSVC (VS 2022 and earlier) only understand `[[gsl::suppress(tag)]]` without the double quotes around `tag`.

For portable code you can use `GSL_SUPPRESS(tag)`.

Expand Down
11 changes: 6 additions & 5 deletions include/gsl/assert
Original file line number Diff line number Diff line change
Expand Up @@ -47,13 +47,14 @@
//
#if defined(__clang__)
#define GSL_SUPPRESS(x) [[gsl::suppress(#x)]]
#else
#if defined(_MSC_VER) && !defined(__INTEL_COMPILER) && !defined(__NVCC__)
#define GSL_SUPPRESS(x) [[gsl::suppress(x)]]
#elif defined(_MSC_VER) && _MSC_VER >= 1950
// Visual Studio versions after 2022 (_MSC_VER > 1944) support the justification message.
#define GSL_SUPPRESS(x) [[gsl::suppress(#x)]]
#elif defined(_MSC_VER) && !defined(__INTEL_COMPILER) && !defined(__NVCC__)
#define GSL_SUPPRESS(x) [[gsl::suppress(#x)]]
Copy link
Contributor

Choose a reason for hiding this comment

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

The PR description says that the macro changed for new VS, it does not mention a change for old VS.

Also the documentation change in this PR says that

Older versions of MSVC (VS 2022 and earlier) only understand [[gsl::suppress(tag)]] without the double quotes around tag.

Yet this here changes the behaviour for old VS.
But I am surprised to see that it works on VS 2022 on my installation. So the documentation and the code change leave me confused.

Copy link
Member Author

Choose a reason for hiding this comment

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

Good catch, that's wrong; line 54 should be suppress(x).

As for why it works on VS 2022...

I suspect it "works" in the sense that the program compiles without errors, but the suppression will not actually be picked up by code analysis. This is because, for older compilers (pre 19.50 toolset), the gsl::suppress argument was just stored and never checked (it could be gsl::suppress(<literally-anything>)) then during code analysis, the argument is actually analyzed and if it is garbage, it would be silently thrown out.

The latest version of the compiler does a little better and the front-end will report some problems with a bad gsl::suppress, but it still won't report everything.

I'll create an issue to get the behavior fixed for old VS versions.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry, I was unprecise. I tested on VS2022 17.14.21, toolset v143.

I have a line of code that triggers a code analysis warning. Above that line I wrote GSL_SUPPRESS(foo).
With #define GSL_SUPPRESS(x) I get the code analysis warning. That is expected as there is no suppression active.
With #define GSL_SUPPRESS(x) [[gsl::suppress(x)]] the warning is suppressed.
With #define GSL_SUPPRESS(x) [[gsl::suppress(#x)]] the warning is also suppressed.

Maybe latest VS2022 already has some change that allows [[gsl::suppress(foo)]] and [[gsl::suppress("foo")]]?

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah, I see. I was misremembering the timing that things got merged.

For the VS2022 17.13 release (toolset v143, compiler 19.43), we added support for gsl::suppress("x") in addition to the existing gsl::suppress(x). Before that, the compiler silently ignored gsl::suppress("x"). Example: https://godbolt.org/z/jb6TG5Y6o

A new Visual Studio version will soon be available that deprecates the old syntax for gsl::suppress. Customers will now get a C4875 diagnostic on suppressions that look like gsl::suppress(x) urging them to use gsl::suppress("x") instead.

When I wrote this PR, C4875 was going to go out with VS2026 18.0 (toolset v150, compiler 19.50). Unfortunately, it was reverted at the last second and is now slotted to go out with the next compiler release. According to our internal tracking, it is part of toolset v151, which I guess will be made available for preview starting Q1 next year (https://devblogs.microsoft.com/cppblog/new-release-cadence-and-support-lifecycle-for-msvc-build-tools/

#else
#define GSL_SUPPRESS(x)
#endif // _MSC_VER
#endif // __clang__
#endif // defined(__clang__)

#if defined(__clang__) || defined(__GNUC__)
#define GSL_LIKELY(x) __builtin_expect(!!(x), 1)
Expand Down