rsmith 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"); ---------------- aaron.ballman wrote: > rsmith wrote: > > aaron.ballman wrote: > > > aaron.ballman wrote: > > > > 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? > > > I suppose one option would be to look at what version of MSVC we're > > > trying to be compatible with to see if that's a version that supports > > > `/std:c11` and only emit this diagnostic in that case, but tbh, that > > > feels like it'll lead to confusing diagnostic behavior (esp given that we > > > default to ms compatibility mode silently when you build Clang with MSVC > > > on Windows). > > > > > > Given that MSVC does support `_Static_assert` when you enable C11 or > > > later language mode, I'm inclined to warn on this construct by default. > > > > > > WDYT? > > Well, it's good to see that they've made progress, but it [looks > > like](https://godbolt.org/z/YfEhGW) their `<assert.h>` still doesn't > > `#define static_assert`, so I think we still don't have an actionable > > warning we can produce here. We can't reasonably tell people to include > > `<assert.h>` (as this patch does) because that doesn't work. And it doesn't > > seem reasonable to tell people to use `_Static_assert` instead, if they > > actually have included `<assert.h>`. (I don't think we want to encourage > > people to use `_Static_assert` instead of `<assert.h>` + `static_assert`.) > > > > So I don't think MSVC adding support for `_Static_assert` really changes > > anything here -- until their `<assert.h>` works, or we find some good way > > to detect whether it was properly included, this warning will fire on both > > correct code and incorrect code, which doesn't seem all that useful. > > And it doesn't seem reasonable to tell people to use _Static_assert > > instead, if they actually have included <assert.h>. (I don't think we want > > to encourage people to use _Static_assert instead of <assert.h> + > > static_assert.) > > Ideally, yes. But this isn't ideal -- we produce no diagnostic for this > nonconforming extension and that's causing pain in practice. As an example of > where I ran into this: I had a header file that was shared between C and > (mostly) C++ code and added a `static_assert` to it but forgot to add > `#include <assert.h>`. This compiled great in MSVC and clang-cl, but when > compiled with clang on CI is when I finally found the issue. e.g., like this: > https://godbolt.org/z/cs8YGb > > If I had to pick between behaviors, I think I'd prefer pushing people towards > using `_Static_assert` even if `assert.h` is included over silently accepting > a nonconforming extension in pedantic mode. > > Rather than trying to see what header guard was used by `<assert.h>`, > couldn't we assume that if `assert` is defined as a macro then `<assert.h>` > must have been included (or the user triggered UB and gets what they get)? So > the logic could be: only diagnose use of the `static_assert` (keyword) in C > mode if `assert` is not defined? > > > We can't reasonably tell people to include <assert.h> (as this patch does) > > because that doesn't work. > > But it does (as far as the user is concerned)? In MS compatibility mode, > `static_assert` is always accepted, so the include isn't strictly necessary > but isn't harmful either. Outside of MS compatibility mode, the include is > required to spell it `static_assert` because we won't treat it as a keyword. > If we go with the "only diagnose when `assert` is not defined as a macro" > approach, it would give us the desired behavior here. > we produce no diagnostic for this nonconforming extension Well, I'd argue that it's not a non-conforming extension so much as it's a different language mode that has different rules. We don't accept `static_assert` if `-fms-compatibility` is disabled. I don't think conformance to ISO C has any bearing here, because you're not compiling the input in ISO C mode. > So the logic could be: only diagnose use of the `static_assert` (keyword) in > C mode if `assert` is not defined? Interesting, I'd not considered that. I think that could work well enough. This approach wouldn't work when compiling preprocessed output, though perhaps we could fix that too, by making the preprocessor implicitly inject a `#define static_assert _Static_assert` if it sees a `#define` of `assert` in MS compatibility mode. We'd need to keep the `static_assert` keyword around for compatibility with code that uses it without a suitable `#include`, but we could then warn on it unconditionally, as this patch does. > > We can't reasonably tell people to include <assert.h> (as this patch does) > > because that doesn't work. > But it does (as far as the user is concerned)? As far as the user is concerned, if their source file already includes `<assert.h>` and they get a warning that says "you need to include `<assert.h>`", they would reasonably conclude that that's a compiler bug. I think we need to avoid that possibility. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D95396/new/ https://reviews.llvm.org/D95396 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
