rsmith added a comment. FYI, https://reviews.llvm.org/D17444 captures some of the history here.
================ 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: > 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. 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
