[PATCH] D95396: Improve static_assert/_Static_assert diagnostics

Fri Jan 29 04:57:45 PST 2021

aaron.ballman added inline comments.

================
Comment at: clang/lib/Parse/ParseDeclCXX.cpp:874-876
+    if (!getLangOpts().CPlusPlus)
+      Diag(Tok, diag::warn_cxx_static_assert_in_c)
+          << FixItHint::CreateReplacement(Tok.getLocation(), "_Static_assert");
----------------
rsmith wrote:
> I don't think this diagnostic is useful as-is: on Windows, including `<assert.h>` doesn't help because it doesn't `#define static_assert`. And people hitting this also can't switch to using `_Static_assert`, because MSVC doesn't provide it, only `static_assert`.
> 
> If we want to warn here, we could perhaps check whether `<assert.h>` has been included, but getting that check correct across PCH / modules is not straightforward. (If we knew what include guard the CRT's `assert.h` used (if any), I guess we could check whether that's defined, but that'd be a bit of a hack.) But I'm somewhat inclined to think we don't have a good way to distinguish between the good cases and the bad ones, so we shouldn't warn. Hopefully MS will fix their CRT at some point and we can stop providing this compatibility hack entirely (or start warning on it by default).
Are you sure they don't support `_Static_assert` yet? I seem to be able to use it fine: https://godbolt.org/z/vG47he

That said, this does appear to be only available in newer versions of MSVC, so perhaps you're correct about the diagnostic being a bit unhelpful. My primary concern is that use of `static_assert` in C is a nonconforming extension and we default to `-fms-compatibility` on Windows when Clang is built by MSVC. So it's not difficult to accidentally run into this, but the only warning we give on it with `-Weverything -pedantic` is how it's not compatible with C++98.

WDYT?

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D95396/new/

https://reviews.llvm.org/D95396