`GSL_SUPPRESS(y)` expands to `[[gsl::suppress("x")]]` for clang

[edgchen1]

Here's the definition of GSL_SUPPRESS:

https://github.com/microsoft/GSL/blob/4300304ef24c247b3db0255763f46b9f95c3a83d/include/gsl/assert#L48-L50

This always expands to [[gsl::suppress("x")]] because the "x" string literal won't be replaced.

We probably want something like this instead:

#define GSL_SUPPRESS(x) [[gsl::suppress( #x )]]

[dmitrykobets-msft]

@edgchen1 good catch. If you'd like to tackle this one you can assign the issue to yourself.

note: whoever ends up implementing this will want to add a test-case for the suppression behavior.

[edgchen1]

Any suggestions on how to add a test for it? I'm not too familiar with the existing test setup, but from a quick look it wasn't obvious to me.

[beinhaerter]

Good question. I was thinking about compiling with -Werror and either triggering or suppressing the warning. I googled a bit and found https://stackoverflow.com/questions/30155619/expected-build-failure-tests-in-cmake https://ibob.bg/blog/2022/10/04/testing-build-failure-with-cmake/#the-final-solution https://github.com/iboB/icm/blob/master/icm_build_failure_testing.cmake

I am not a cmake expert and right now I don't want to invest more time to try to implement the tests.

From a look into the test code here I found that there are some #ifdef CONFIRM_COMPILATION_ERRORS, but I don't see how and where that value is defined. And even iif it was, it would cover multiple test cases and the test would succeed if any of these tests failed to compile. I would expect the test to succeed if all of these tests fail to compile.

I hope that dmitrykobets-msft will accept a fix even without an extra test and that he comes up with an idea how to get the test case working. When the test case is implemented, also the #ifdef CONFIRM_COMPILATION_ERRORS could be changed into individual test cases.

[dmitrykobets-msft]

Fair enough; looks like we will want some form of compilation-failure testing, and currently CONFIRM_COMPILATION_ERRORS is non-functional (and has been for a while); this is something we'll want to address in the future. The change suggested in this issue is small and simple enough, and touches an area that is not expected to change anytime soon, so we can skip adding automated testing.

[edgchen1]

Thanks for the input @beinhaerter and @dmitrykobets-msft.

I created a small PR (#1133) with just the change in this issue.