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: > 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. 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
