[PATCH] D95396: Improve static_assert/_Static_assert diagnostics

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added a comment.

@rnk / @thakis Can you take a look at this and see if you're happy with this "defining `assert` implicitly defines `static_assert`" approach?



================
Comment at: clang/include/clang/Basic/DiagnosticParseKinds.td:427-429
+def warn_cxx_static_assert_in_c : Warning<
+  "'static_assert' is spelled '_Static_assert' in C; consider including "
+  "<assert.h>">, InGroup<DiagGroup<"static-assert-extension">>;
----------------
Perhaps:

```
def ext_ms_static_assert : ExtWarn<
  "use of 'static_assert' without inclusion of <assert.h> is a Microsoft extension">,
  "<assert.h>">, InGroup<MicrosoftStaticAssert>;
```

I think `MicrosoftStaticAssert` should also be a subgroup of the `Microsoft` (`-Wmicrosoft`) warning group.


================
Comment at: clang/lib/Lex/PPDirectives.cpp:2884
+    Tok.setKind(tok::kw__Static_assert);
+    Tok.setIdentifierInfo(getIdentifierInfo("_Static_assert"));
+    MI->AddTokenToBody(Tok);
----------------
Do we need to give the expansion token a source location? What do diagnostics pointing at this token look like?


================
Comment at: clang/lib/Parse/ParseDeclCXX.cpp:881
+    // MSVC's assert.h does not provide such a macro definition.
+    if (!getLangOpts().CPlusPlus && !PP.isMacroDefined("assert"))
+      Diag(Tok, diag::warn_cxx_static_assert_in_c)
----------------
I don't think we need the `isMacroDefined` check here; we will have a `static_assert` macro in that case, so we won't see a `kw_static_assert` token. Checking for `assert` being defined means we won't warn on this, and we should:

```
#include <assert.h>
#undef static_assert
static_assert(1, "");
```


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

https://reviews.llvm.org/D95396