[PATCH] D95396: Improve static_assert/_Static_assert diagnostics

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman closed this revision.
aaron.ballman added a comment.

In D95396#2598932 , @rsmith wrote:

> Looks like `test/FixIt/fixit-static-assert.cpp` is failing in Phabricator's 
> pre-merge checks: B91556 . Please take a 
> look at that before landing this; I think there's a decent chance that it's 
> indicative of a real problem.

The issue was that I was using `Extension` instead of `ExtWarn` for the 
`'static_assert' with no message` warnings -- I changed it to use `ExtWarn` and 
fixed up the new tests. If you prefer I use `Extension` instead, I'm happy to 
do that and change the fixit test instead.

I've commit in 8da090381d567d0ec555840f6b2a651d2997e4b3 
, thank 
you for the reviews!


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

[PATCH] D95396: Improve static_assert/_Static_assert diagnostics

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

rsmith added a comment.

Looks like `test/FixIt/fixit-static-assert.cpp` is failing in Phabricator's 
pre-merge checks: B91556 . Please take a look 
at that before landing this; I think there's a decent chance that it's 
indicative of a real problem.


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

[PATCH] D95396: Improve static_assert/_Static_assert diagnostics

[Reid Kleckner via Phabricator via cfe-commits]

rnk accepted this revision.
rnk added a comment.
This revision is now accepted and ready to land.

lgtm, but please make sure that Richard is happy


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

[PATCH] D95396: Improve static_assert/_Static_assert diagnostics

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman updated this revision to Diff 327426.
aaron.ballman added a comment.

Address review comments from @rnk

Only define the `static_assert` macro if one is not already defined and test 
the expected behavior.


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

https://reviews.llvm.org/D95396

Files:
  clang/include/clang/Basic/DiagnosticGroups.td
  clang/include/clang/Basic/DiagnosticParseKinds.td
  clang/lib/Lex/PPDirectives.cpp
  clang/lib/Parse/ParseDeclCXX.cpp
  clang/test/Parser/static_assert.c
  clang/test/Preprocessor/static_assert-already-defined.c
  clang/test/Preprocessor/static_assert.c
  clang/test/Sema/static-assert.c
  clang/test/SemaCXX/static-assert.cpp

Index: clang/test/SemaCXX/static-assert.cpp
===
--- clang/test/SemaCXX/static-assert.cpp
+++ clang/test/SemaCXX/static-assert.cpp
@@ -1,4 +1,4 @@
-// RUN: %clang_cc1 -fsyntax-only -verify %s -std=c++11 -triple=x86_64-linux-gnu
+// RUN: %clang_cc1 -fsyntax-only -verify %s -std=c++11 -pedantic -triple=x86_64-linux-gnu
 
 int f(); // expected-note {{declared here}}
 
Index: clang/test/Sema/static-assert.c
===
--- clang/test/Sema/static-assert.c
+++ clang/test/Sema/static-assert.c
@@ -1,5 +1,5 @@
 // RUN: %clang_cc1 -std=c11 -fsyntax-only -verify %s
-// RUN: %clang_cc1 -fms-compatibility -DMS -fsyntax-only -verify %s
+// RUN: %clang_cc1 -fms-compatibility -DMS -fsyntax-only -verify=expected,ms %s
 // RUN: %clang_cc1 -std=c99 -pedantic -fsyntax-only -verify=expected,ext %s
 // RUN: %clang_cc1 -xc++ -std=c++11 -pedantic -fsyntax-only -verify=expected,ext,cxx %s
 
@@ -13,7 +13,7 @@
// ext-warning {{'_Static_assert' is a C11 extension}}
 
 #ifdef MS
-static_assert(1, "1 is nonzero");
+static_assert(1, "1 is nonzero"); // ms-warning {{use of 'static_assert' without inclusion of  is a Microsoft extension}}
 #endif
 
 void foo(void) {
@@ -21,7 +21,7 @@
   _Static_assert(0, "0 is nonzero"); // expected-error {{static_assert failed "0 is nonzero"}} \
  // ext-warning {{'_Static_assert' is a C11 extension}}
 #ifdef MS
-  static_assert(1, "1 is nonzero");
+  static_assert(1, "1 is nonzero"); // ms-warning {{use of 'static_assert' without}}
 #endif
 }
 
@@ -34,7 +34,7 @@
   _Static_assert(0, "0 is nonzero"); // expected-error {{static_assert failed "0 is nonzero"}} \
  // ext-warning {{'_Static_assert' is a C11 extension}}
 #ifdef MS
-  static_assert(1, "1 is nonzero");
+  static_assert(1, "1 is nonzero"); // ms-warning {{use of 'static_assert' without}}
 #endif
 };
 
@@ -58,3 +58,15 @@
// ext-warning 3 {{'_Static_assert' is a C11 extension}}
 typedef UNION(float, 0.5f) U4; // expected-error {{expected a type}} \
// ext-warning 3 {{'_Static_assert' is a C11 extension}}
+
+// After defining the assert macro in MS-compatibility mode, we should
+// no longer warn about including  under the assumption the
+// user already did that.
+#ifdef MS
+#define assert(expr)
+static_assert(1, "1 is nonzero"); // ok
+
+// But we should still warn if the user did something bonkers.
+#undef static_assert
+static_assert(1, "1 is nonzero"); // ms-warning {{use of 'static_assert' without}}
+#endif
Index: clang/test/Preprocessor/static_assert.c
===
--- /dev/null
+++ clang/test/Preprocessor/static_assert.c
@@ -0,0 +1,12 @@
+// RUN: %clang_cc1 -E -dM %s | FileCheck --strict-whitespace --check-prefix=NOMS %s
+// RUN: %clang_cc1 -fms-compatibility -E -dM %s | FileCheck --strict-whitespace --check-prefix=MS %s
+
+// If the assert macro is defined in MS compatibility mode in C, we
+// automatically inject a macro definition for static_assert. Test that the
+// macro is properly added to the preprocessed output. This allows us to
+// diagonse use of the static_assert keyword when  has not been
+// included while still being able to compile preprocessed code.
+#define assert
+
+MS: #define static_assert _Static_assert
+NOMS-NOT: #define static_assert _Static_assert
Index: clang/test/Preprocessor/static_assert-already-defined.c
===
--- /dev/null
+++ clang/test/Preprocessor/static_assert-already-defined.c
@@ -0,0 +1,23 @@
+// RUN: %clang_cc1 -DFIRST_WAY -E -dM %s | FileCheck --strict-whitespace %s
+// RUN: %clang_cc1 -DFIRST_WAY -fms-compatibility -E -dM %s | FileCheck --strict-whitespace %s
+// RUN: %clang_cc1 -E -dM %s | FileCheck --strict-whitespace %s
+// RUN: %clang_cc1 -fms-compatibility -E -dM %s | FileCheck --strict-whitespace %s
+
+// If the assert macro is defined in MS compatibility mode in C, we
+// automatically inject a macro definition for static_assert. Test that the
+// macro is not added if there is already a definition of 

[PATCH] D95396: Improve static_assert/_Static_assert diagnostics

[Reid Kleckner via Phabricator via cfe-commits]

rnk added a comment.

I guess my only concern is, what happens if MSVC fixes assert.h? Do we need to 
make the implicit `#define static_assert _Static_assert` conditional on the 
absence of any `static_assert` definition?


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

[PATCH] D95396: Improve static_assert/_Static_assert diagnostics

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

Ping.


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

[PATCH] D95396: Improve static_assert/_Static_assert diagnostics

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman updated this revision to Diff 325487.
aaron.ballman added a comment.

Rebasing on ToT.


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

https://reviews.llvm.org/D95396

Files:
  clang/include/clang/Basic/DiagnosticGroups.td
  clang/include/clang/Basic/DiagnosticParseKinds.td
  clang/lib/Lex/PPDirectives.cpp
  clang/lib/Parse/ParseDeclCXX.cpp
  clang/test/Parser/static_assert.c
  clang/test/Preprocessor/static_assert.c
  clang/test/Sema/static-assert.c
  clang/test/SemaCXX/static-assert.cpp

Index: clang/test/SemaCXX/static-assert.cpp
===
--- clang/test/SemaCXX/static-assert.cpp
+++ clang/test/SemaCXX/static-assert.cpp
@@ -1,4 +1,4 @@
-// RUN: %clang_cc1 -fsyntax-only -verify %s -std=c++11 -triple=x86_64-linux-gnu
+// RUN: %clang_cc1 -fsyntax-only -verify %s -std=c++11 -pedantic -triple=x86_64-linux-gnu
 
 int f(); // expected-note {{declared here}}
 
Index: clang/test/Sema/static-assert.c
===
--- clang/test/Sema/static-assert.c
+++ clang/test/Sema/static-assert.c
@@ -1,5 +1,5 @@
 // RUN: %clang_cc1 -std=c11 -fsyntax-only -verify %s
-// RUN: %clang_cc1 -fms-compatibility -DMS -fsyntax-only -verify %s
+// RUN: %clang_cc1 -fms-compatibility -DMS -fsyntax-only -verify=expected,ms %s
 // RUN: %clang_cc1 -std=c99 -pedantic -fsyntax-only -verify=expected,ext %s
 // RUN: %clang_cc1 -xc++ -std=c++11 -pedantic -fsyntax-only -verify=expected,ext,cxx %s
 
@@ -13,7 +13,7 @@
// ext-warning {{'_Static_assert' is a C11 extension}}
 
 #ifdef MS
-static_assert(1, "1 is nonzero");
+static_assert(1, "1 is nonzero"); // ms-warning {{use of 'static_assert' without inclusion of  is a Microsoft extension}}
 #endif
 
 void foo(void) {
@@ -21,7 +21,7 @@
   _Static_assert(0, "0 is nonzero"); // expected-error {{static_assert failed "0 is nonzero"}} \
  // ext-warning {{'_Static_assert' is a C11 extension}}
 #ifdef MS
-  static_assert(1, "1 is nonzero");
+  static_assert(1, "1 is nonzero"); // ms-warning {{use of 'static_assert' without}}
 #endif
 }
 
@@ -34,7 +34,7 @@
   _Static_assert(0, "0 is nonzero"); // expected-error {{static_assert failed "0 is nonzero"}} \
  // ext-warning {{'_Static_assert' is a C11 extension}}
 #ifdef MS
-  static_assert(1, "1 is nonzero");
+  static_assert(1, "1 is nonzero"); // ms-warning {{use of 'static_assert' without}}
 #endif
 };
 
@@ -58,3 +58,15 @@
// ext-warning 3 {{'_Static_assert' is a C11 extension}}
 typedef UNION(float, 0.5f) U4; // expected-error {{expected a type}} \
// ext-warning 3 {{'_Static_assert' is a C11 extension}}
+
+// After defining the assert macro in MS-compatibility mode, we should
+// no longer warn about including  under the assumption the
+// user already did that.
+#ifdef MS
+#define assert(expr)
+static_assert(1, "1 is nonzero"); // ok
+
+// But we should still warn if the user did something bonkers.
+#undef static_assert
+static_assert(1, "1 is nonzero"); // ms-warning {{use of 'static_assert' without}}
+#endif
Index: clang/test/Preprocessor/static_assert.c
===
--- /dev/null
+++ clang/test/Preprocessor/static_assert.c
@@ -0,0 +1,12 @@
+// RUN: %clang_cc1 -E -dM %s | FileCheck --strict-whitespace --check-prefix=NOMS %s
+// RUN: %clang_cc1 -fms-compatibility -E -dM %s | FileCheck --strict-whitespace --check-prefix=MS %s
+
+// If the assert macro is defined in MS compatibility mode in C, we
+// automatically inject a macro definition for static_assert. Test that the
+// macro is properly added to the preprocessed output. This allows us to
+// diagonse use of the static_assert keyword when  has not been
+// included while still being able to compile preprocessed code.
+#define assert
+
+MS: #define static_assert _Static_assert
+NOMS-NOT: #define static_assert _Static_assert
Index: clang/test/Parser/static_assert.c
===
--- /dev/null
+++ clang/test/Parser/static_assert.c
@@ -0,0 +1,45 @@
+// RUN: %clang_cc1 -fsyntax-only -std=c2x -DTEST_SPELLING -verify=c2x %s
+// RUN: %clang_cc1 -fsyntax-only -std=c2x -DTEST_SPELLING -fms-compatibility -verify=c2x-ms %s
+// RUN: %clang_cc1 -fsyntax-only -std=c2x -Wpre-c2x-compat -verify=c2x-compat %s
+// RUN: %clang_cc1 -fsyntax-only -std=c99 -verify=c99 %s
+// RUN: %clang_cc1 -fsyntax-only -std=c99 -pedantic -verify=c99-pedantic %s
+// RUN: %clang_cc1 -fsyntax-only -std=c++17 -verify=cxx17 -x c++ %s
+// RUN: %clang_cc1 -fsyntax-only -std=c++17 -pedantic -verify=cxx17-pedantic -x c++ %s
+// RUN: %clang_cc1 -fsyntax-only -std=c++98 -verify=cxx98 -x c++ %s
+// RUN: %clang_cc1 -fsyntax-only -std=c++98 -pedantic -verify=cxx98-pedantic -x c++ %s
+// RUN: %clang_cc1 -fsyntax-only -std=c++17 

[PATCH] D95396: Improve static_assert/_Static_assert diagnostics

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: clang/lib/Lex/PPDirectives.cpp:2884
+Tok.setKind(tok::kw__Static_assert);
+Tok.setIdentifierInfo(getIdentifierInfo("_Static_assert"));
+MI->AddTokenToBody(Tok);

rsmith wrote:
> Do we need to give the expansion token a source location? What do diagnostics 
> pointing at this token look like?
Given:
```
#define assert

// This is a comment
int i = 0;
static_assert(0, "test");
```
I get:
```
F:\llvm-project>llvm\out\build\x64-Debug\bin\clang.exe -fsyntax-only "F:\Aaron B
allman\Desktop\test.c"
F:\Aaron Ballman\Desktop\test.c:5:1: error: static_assert failed "test"
static_assert(0, "test");
^ ~
F:\Aaron Ballman\Desktop\test.c:5:1: note: expanded from macro 'static_assert'
static_assert(0, "test");
^
1 error generated.
```
which is a bit funky due to the note. However, I'm not certain what source 
location to give it. If I give it the location of the `assert` identifier, then 
I get output like:
```
F:\llvm-project>llvm\out\build\x64-Debug\bin\clang.exe -fsyntax-only "F:\Aaron B
allman\Desktop\test.c"
F:\Aaron Ballman\Desktop\test.c:5:1: error: static_assert failed "test"
static_assert(0, "test");
^ ~
F:\Aaron Ballman\Desktop\test.c:1:9: note: expanded from macro 'static_assert'
#define assert
^
1 error generated.
```
which seems even more mysterious.


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

[PATCH] D95396: Improve static_assert/_Static_assert diagnostics

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman updated this revision to Diff 325029.
aaron.ballman marked 2 inline comments as done.
aaron.ballman added a comment.

Updating based on review feedback.


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

https://reviews.llvm.org/D95396

Files:
  clang/include/clang/Basic/DiagnosticGroups.td
  clang/include/clang/Basic/DiagnosticParseKinds.td
  clang/lib/Lex/PPDirectives.cpp
  clang/lib/Parse/ParseDeclCXX.cpp
  clang/test/Parser/static_assert.c
  clang/test/Preprocessor/static_assert.c
  clang/test/Sema/static-assert.c
  clang/test/SemaCXX/static-assert.cpp

Index: clang/test/SemaCXX/static-assert.cpp
===
--- clang/test/SemaCXX/static-assert.cpp
+++ clang/test/SemaCXX/static-assert.cpp
@@ -1,4 +1,4 @@
-// RUN: %clang_cc1 -fsyntax-only -verify %s -std=c++11 -triple=x86_64-linux-gnu
+// RUN: %clang_cc1 -fsyntax-only -verify %s -std=c++11 -pedantic -triple=x86_64-linux-gnu
 
 int f(); // expected-note {{declared here}}
 
Index: clang/test/Sema/static-assert.c
===
--- clang/test/Sema/static-assert.c
+++ clang/test/Sema/static-assert.c
@@ -1,5 +1,5 @@
 // RUN: %clang_cc1 -std=c11 -fsyntax-only -verify %s
-// RUN: %clang_cc1 -fms-compatibility -DMS -fsyntax-only -verify %s
+// RUN: %clang_cc1 -fms-compatibility -DMS -fsyntax-only -verify=expected,ms %s
 // RUN: %clang_cc1 -std=c99 -pedantic -fsyntax-only -verify=expected,ext %s
 // RUN: %clang_cc1 -xc++ -std=c++11 -pedantic -fsyntax-only -verify=expected,ext,cxx %s
 
@@ -13,7 +13,7 @@
// ext-warning {{'_Static_assert' is a C11 extension}}
 
 #ifdef MS
-static_assert(1, "1 is nonzero");
+static_assert(1, "1 is nonzero"); // ms-warning {{use of 'static_assert' without inclusion of  is a Microsoft extension}}
 #endif
 
 void foo(void) {
@@ -21,7 +21,7 @@
   _Static_assert(0, "0 is nonzero"); // expected-error {{static_assert failed "0 is nonzero"}} \
  // ext-warning {{'_Static_assert' is a C11 extension}}
 #ifdef MS
-  static_assert(1, "1 is nonzero");
+  static_assert(1, "1 is nonzero"); // ms-warning {{use of 'static_assert' without}}
 #endif
 }
 
@@ -34,7 +34,7 @@
   _Static_assert(0, "0 is nonzero"); // expected-error {{static_assert failed "0 is nonzero"}} \
  // ext-warning {{'_Static_assert' is a C11 extension}}
 #ifdef MS
-  static_assert(1, "1 is nonzero");
+  static_assert(1, "1 is nonzero"); // ms-warning {{use of 'static_assert' without}}
 #endif
 };
 
@@ -58,3 +58,15 @@
// ext-warning 3 {{'_Static_assert' is a C11 extension}}
 typedef UNION(float, 0.5f) U4; // expected-error {{expected a type}} \
// ext-warning 3 {{'_Static_assert' is a C11 extension}}
+
+// After defining the assert macro in MS-compatibility mode, we should
+// no longer warn about including  under the assumption the
+// user already did that.
+#ifdef MS
+#define assert(expr)
+static_assert(1, "1 is nonzero"); // ok
+
+// But we should still warn if the user did something bonkers.
+#undef static_assert
+static_assert(1, "1 is nonzero"); // ms-warning {{use of 'static_assert' without}}
+#endif
Index: clang/test/Preprocessor/static_assert.c
===
--- /dev/null
+++ clang/test/Preprocessor/static_assert.c
@@ -0,0 +1,12 @@
+// RUN: %clang_cc1 -E -dM %s | FileCheck --strict-whitespace --check-prefix=NOMS %s
+// RUN: %clang_cc1 -fms-compatibility -E -dM %s | FileCheck --strict-whitespace --check-prefix=MS %s
+
+// If the assert macro is defined in MS compatibility mode in C, we
+// automatically inject a macro definition for static_assert. Test that the
+// macro is properly added to the preprocessed output. This allows us to
+// diagonse use of the static_assert keyword when  has not been
+// included while still being able to compile preprocessed code.
+#define assert
+
+MS: #define static_assert _Static_assert
+NOMS-NOT: #define static_assert _Static_assert
Index: clang/test/Parser/static_assert.c
===
--- /dev/null
+++ clang/test/Parser/static_assert.c
@@ -0,0 +1,45 @@
+// RUN: %clang_cc1 -fsyntax-only -std=c2x -DTEST_SPELLING -verify=c2x %s
+// RUN: %clang_cc1 -fsyntax-only -std=c2x -DTEST_SPELLING -fms-compatibility -verify=c2x-ms %s
+// RUN: %clang_cc1 -fsyntax-only -std=c2x -Wpre-c2x-compat -verify=c2x-compat %s
+// RUN: %clang_cc1 -fsyntax-only -std=c99 -verify=c99 %s
+// RUN: %clang_cc1 -fsyntax-only -std=c99 -pedantic -verify=c99-pedantic %s
+// RUN: %clang_cc1 -fsyntax-only -std=c++17 -verify=cxx17 -x c++ %s
+// RUN: %clang_cc1 -fsyntax-only -std=c++17 -pedantic -verify=cxx17-pedantic -x c++ %s
+// RUN: %clang_cc1 -fsyntax-only -std=c++98 -verify=cxx98 -x c++ %s
+// RUN: %clang_cc1 -fsyntax-only -std=c++98 -pedantic 

[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 "
+  "">, InGroup>;

Perhaps:

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

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 
#undef static_assert
static_assert(1, "");
```


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

[PATCH] D95396: Improve static_assert/_Static_assert diagnostics

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman updated this revision to Diff 324707.
aaron.ballman added a comment.

Updated based on review feedback.


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

https://reviews.llvm.org/D95396

Files:
  clang/include/clang/Basic/DiagnosticGroups.td
  clang/include/clang/Basic/DiagnosticParseKinds.td
  clang/lib/Lex/PPDirectives.cpp
  clang/lib/Parse/ParseDeclCXX.cpp
  clang/test/Parser/static_assert.c
  clang/test/Preprocessor/static_assert.c
  clang/test/Sema/static-assert.c
  clang/test/SemaCXX/static-assert.cpp

Index: clang/test/SemaCXX/static-assert.cpp
===
--- clang/test/SemaCXX/static-assert.cpp
+++ clang/test/SemaCXX/static-assert.cpp
@@ -1,4 +1,4 @@
-// RUN: %clang_cc1 -fsyntax-only -verify %s -std=c++11 -triple=x86_64-linux-gnu
+// RUN: %clang_cc1 -fsyntax-only -verify %s -std=c++11 -pedantic -triple=x86_64-linux-gnu
 
 int f(); // expected-note {{declared here}}
 
Index: clang/test/Sema/static-assert.c
===
--- clang/test/Sema/static-assert.c
+++ clang/test/Sema/static-assert.c
@@ -1,5 +1,5 @@
 // RUN: %clang_cc1 -std=c11 -fsyntax-only -verify %s
-// RUN: %clang_cc1 -fms-compatibility -DMS -fsyntax-only -verify %s
+// RUN: %clang_cc1 -fms-compatibility -DMS -fsyntax-only -verify=expected,ms %s
 // RUN: %clang_cc1 -std=c99 -pedantic -fsyntax-only -verify=expected,ext %s
 // RUN: %clang_cc1 -xc++ -std=c++11 -pedantic -fsyntax-only -verify=expected,ext,cxx %s
 
@@ -13,7 +13,7 @@
// ext-warning {{'_Static_assert' is a C11 extension}}
 
 #ifdef MS
-static_assert(1, "1 is nonzero");
+static_assert(1, "1 is nonzero"); // ms-warning {{'static_assert' is spelled '_Static_assert' in C; consider including }}
 #endif
 
 void foo(void) {
@@ -21,7 +21,7 @@
   _Static_assert(0, "0 is nonzero"); // expected-error {{static_assert failed "0 is nonzero"}} \
  // ext-warning {{'_Static_assert' is a C11 extension}}
 #ifdef MS
-  static_assert(1, "1 is nonzero");
+  static_assert(1, "1 is nonzero"); // ms-warning {{'static_assert' is spelled '_Static_assert'}}
 #endif
 }
 
@@ -34,7 +34,7 @@
   _Static_assert(0, "0 is nonzero"); // expected-error {{static_assert failed "0 is nonzero"}} \
  // ext-warning {{'_Static_assert' is a C11 extension}}
 #ifdef MS
-  static_assert(1, "1 is nonzero");
+  static_assert(1, "1 is nonzero"); // ms-warning {{'static_assert' is spelled '_Static_assert'}}
 #endif
 };
 
@@ -58,3 +58,11 @@
// ext-warning 3 {{'_Static_assert' is a C11 extension}}
 typedef UNION(float, 0.5f) U4; // expected-error {{expected a type}} \
// ext-warning 3 {{'_Static_assert' is a C11 extension}}
+
+// After defining the assert macro in MS-compatibility mode, we should
+// no longer warn about including  under the assumption the
+// user already did that.
+#ifdef MS
+#define assert(expr)
+static_assert(1, "1 is nonzero"); // ok
+#endif
Index: clang/test/Preprocessor/static_assert.c
===
--- /dev/null
+++ clang/test/Preprocessor/static_assert.c
@@ -0,0 +1,12 @@
+// RUN: %clang_cc1 -E -dM %s | FileCheck --strict-whitespace --check-prefix=NOMS %s
+// RUN: %clang_cc1 -fms-compatibility -E -dM %s | FileCheck --strict-whitespace --check-prefix=MS %s
+
+// If the assert macro is defined in MS compatibility mode in C, we
+// automatically inject a macro definition for static_assert. Test that the
+// macro is properly added to the preprocessed output. This allows us to
+// diagonse use of the static_assert keyword when  has not been
+// included while still being able to compile preprocessed code.
+#define assert
+
+MS: #define static_assert _Static_assert
+NOMS-NOT: #define static_assert _Static_assert
Index: clang/test/Parser/static_assert.c
===
--- /dev/null
+++ clang/test/Parser/static_assert.c
@@ -0,0 +1,45 @@
+// RUN: %clang_cc1 -fsyntax-only -std=c2x -DTEST_SPELLING -verify=c2x %s
+// RUN: %clang_cc1 -fsyntax-only -std=c2x -DTEST_SPELLING -fms-compatibility -verify=c2x-ms %s
+// RUN: %clang_cc1 -fsyntax-only -std=c2x -Wpre-c2x-compat -verify=c2x-compat %s
+// RUN: %clang_cc1 -fsyntax-only -std=c99 -verify=c99 %s
+// RUN: %clang_cc1 -fsyntax-only -std=c99 -pedantic -verify=c99-pedantic %s
+// RUN: %clang_cc1 -fsyntax-only -std=c++17 -verify=cxx17 -x c++ %s
+// RUN: %clang_cc1 -fsyntax-only -std=c++17 -pedantic -verify=cxx17-pedantic -x c++ %s
+// RUN: %clang_cc1 -fsyntax-only -std=c++98 -verify=cxx98 -x c++ %s
+// RUN: %clang_cc1 -fsyntax-only -std=c++98 -pedantic -verify=cxx98-pedantic -x c++ %s
+// RUN: %clang_cc1 -fsyntax-only -std=c++17 -Wc++98-c++11-c++14-compat -verify=cxx17-compat -x c++ %s
+
+// c99-no-diagnostics
+// cxx17-no-diagnostics
+// cxx98-no-diagnostics

[PATCH] D95396: Improve static_assert/_Static_assert diagnostics

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

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 
> > > > > `` 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 `` 
> > > > > 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 `` 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 
> > `` (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 ``. (I don't think we want to encourage 
> > people to use `_Static_assert` instead of `` + `static_assert`.)
> > 
> > So I don't think MSVC adding support for `_Static_assert` really changes 
> > anything here -- until their `` 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 . (I don't think we want 
> > to encourage people to use _Static_assert instead of  + 
> > 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 `. 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 ``, 
> couldn't we assume that if `assert` is defined as a macro then `` 
> 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  (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 

[PATCH] D95396: Improve static_assert/_Static_assert diagnostics

[Aaron Ballman via Phabricator via cfe-commits]

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 
> > > > `` 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 `` 
> > > > 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 `` 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 `` (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 
> ``. (I don't think we want to encourage people to use 
> `_Static_assert` instead of `` + `static_assert`.)
> 
> So I don't think MSVC adding support for `_Static_assert` really changes 
> anything here -- until their `` 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 . (I don't think we want to 
> encourage people to use _Static_assert instead of  + 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 
`. 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 ``, couldn't 
we assume that if `assert` is defined as a macro then `` 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  (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 

[PATCH] D95396: Improve static_assert/_Static_assert diagnostics

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

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 
> > > `` 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 `` 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 `` 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 `` (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 
``. (I don't think we want to encourage people to use 
`_Static_assert` instead of `` + `static_assert`.)

So I don't think MSVC adding support for `_Static_assert` really changes 
anything here -- until their `` 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

[PATCH] D95396: Improve static_assert/_Static_assert diagnostics

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman marked 2 inline comments as not done.
aaron.ballman added a comment.

Ping.


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

[PATCH] D95396: Improve static_assert/_Static_assert diagnostics

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman marked an inline comment as not done.
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");

aaron.ballman wrote:
> rsmith wrote:
> > I don't think this diagnostic is useful as-is: on Windows, including 
> > `` 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 `` 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?


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

[PATCH] D95396: Improve static_assert/_Static_assert diagnostics

[Aaron Ballman via Phabricator via cfe-commits]

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 
> `` 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 `` 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

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D95396: Improve static_assert/_Static_assert diagnostics

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

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");

I don't think this diagnostic is useful as-is: on Windows, including 
`` 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 `` 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).


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

[PATCH] D95396: Improve static_assert/_Static_assert diagnostics

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman updated this revision to Diff 319862.
aaron.ballman added a comment.

Rebasing.


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

https://reviews.llvm.org/D95396

Files:
  clang/include/clang/Basic/DiagnosticGroups.td
  clang/include/clang/Basic/DiagnosticParseKinds.td
  clang/lib/Parse/ParseDeclCXX.cpp
  clang/test/Parser/static_assert.c
  clang/test/Sema/static-assert.c
  clang/test/SemaCXX/static-assert.cpp

Index: clang/test/SemaCXX/static-assert.cpp
===
--- clang/test/SemaCXX/static-assert.cpp
+++ clang/test/SemaCXX/static-assert.cpp
@@ -1,4 +1,4 @@
-// RUN: %clang_cc1 -fsyntax-only -verify %s -std=c++11 -triple=x86_64-linux-gnu
+// RUN: %clang_cc1 -fsyntax-only -verify %s -std=c++11 -pedantic -triple=x86_64-linux-gnu
 
 int f(); // expected-note {{declared here}}
 
Index: clang/test/Sema/static-assert.c
===
--- clang/test/Sema/static-assert.c
+++ clang/test/Sema/static-assert.c
@@ -1,5 +1,5 @@
 // RUN: %clang_cc1 -std=c11 -fsyntax-only -verify %s
-// RUN: %clang_cc1 -fms-compatibility -DMS -fsyntax-only -verify %s
+// RUN: %clang_cc1 -fms-compatibility -DMS -fsyntax-only -verify=expected,ms %s
 // RUN: %clang_cc1 -std=c99 -pedantic -fsyntax-only -verify=expected,ext %s
 // RUN: %clang_cc1 -xc++ -std=c++11 -pedantic -fsyntax-only -verify=expected,ext,cxx %s
 
@@ -13,7 +13,7 @@
// ext-warning {{'_Static_assert' is a C11 extension}}
 
 #ifdef MS
-static_assert(1, "1 is nonzero");
+static_assert(1, "1 is nonzero"); // ms-warning {{'static_assert' is spelled '_Static_assert' in C; consider including }}
 #endif
 
 void foo(void) {
@@ -21,7 +21,7 @@
   _Static_assert(0, "0 is nonzero"); // expected-error {{static_assert failed "0 is nonzero"}} \
  // ext-warning {{'_Static_assert' is a C11 extension}}
 #ifdef MS
-  static_assert(1, "1 is nonzero");
+  static_assert(1, "1 is nonzero"); // ms-warning {{'static_assert' is spelled '_Static_assert'}}
 #endif
 }
 
@@ -34,7 +34,7 @@
   _Static_assert(0, "0 is nonzero"); // expected-error {{static_assert failed "0 is nonzero"}} \
  // ext-warning {{'_Static_assert' is a C11 extension}}
 #ifdef MS
-  static_assert(1, "1 is nonzero");
+  static_assert(1, "1 is nonzero"); // ms-warning {{'static_assert' is spelled '_Static_assert'}}
 #endif
 };
 
Index: clang/test/Parser/static_assert.c
===
--- /dev/null
+++ clang/test/Parser/static_assert.c
@@ -0,0 +1,45 @@
+// RUN: %clang_cc1 -fsyntax-only -std=c2x -DTEST_SPELLING -verify=c2x %s
+// RUN: %clang_cc1 -fsyntax-only -std=c2x -DTEST_SPELLING -fms-compatibility -verify=c2x-ms %s
+// RUN: %clang_cc1 -fsyntax-only -std=c2x -Wc89-c99-c11-c17-compat -verify=c2x-compat %s
+// RUN: %clang_cc1 -fsyntax-only -std=c99 -verify=c99 %s
+// RUN: %clang_cc1 -fsyntax-only -std=c99 -pedantic -verify=c99-pedantic %s
+// RUN: %clang_cc1 -fsyntax-only -std=c++17 -verify=cxx17 -x c++ %s
+// RUN: %clang_cc1 -fsyntax-only -std=c++17 -pedantic -verify=cxx17-pedantic -x c++ %s
+// RUN: %clang_cc1 -fsyntax-only -std=c++98 -verify=cxx98 -x c++ %s
+// RUN: %clang_cc1 -fsyntax-only -std=c++98 -pedantic -verify=cxx98-pedantic -x c++ %s
+// RUN: %clang_cc1 -fsyntax-only -std=c++17 -Wc++98-c++11-c++14-compat -verify=cxx17-compat -x c++ %s
+
+// c99-no-diagnostics
+// cxx17-no-diagnostics
+// cxx98-no-diagnostics
+
+#ifdef TEST_SPELLING
+// Only test the C++ spelling in C mode in some of the tests, to reduce the
+// amount of diagnostics to have to check. This spelling is allowed in MS-
+// compatibility mode in C, but otherwise produces errors.
+static_assert(1, ""); // c2x-error {{expected parameter declarator}} \
+  // c2x-error {{expected ')'}} \
+  // c2x-note {{to match this '('}} \
+  // c2x-warning {{type specifier missing, defaults to 'int'}} \
+  // c2x-ms-warning {{'static_assert' is spelled '_Static_assert' in C; consider including }}
+#endif
+
+// We support _Static_assert as an extension in older C modes and in all C++
+// modes, but only as a pedantic warning.
+_Static_assert(1, ""); // c99-pedantic-warning {{'_Static_assert' is a C11 extension}} \
+   // cxx17-pedantic-warning {{'_Static_assert' is a C11 extension}} \
+   // cxx98-pedantic-warning {{'_Static_assert' is a C11 extension}}
+
+// _Static_assert without a message has more complex diagnostic logic:
+//   * In C++17 or C2x mode, it's supported by default.
+//   * But there is a special compat warning flag to warn about portability to
+// older standards.
+//   * In older standard pedantic modes, warn about supporting without a
+// message as an extension.
+_Static_assert(1); // c99-pedantic-warning {{'_Static_assert' with no 

[PATCH] D95396: Improve static_assert/_Static_assert diagnostics

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman created this revision.
aaron.ballman added reviewers: rsmith, jyknight.
aaron.ballman requested review of this revision.

I noticed that our diagnostics relating to static assertions are a bit 
confused. For instance, when in MS compatibility mode in C (where we accept 
`static_assert` even without including ``), we would fail to warn the 
user that they were using the wrong spelling (even in pedantic mode), we were 
missing a compatibility warning about using `_Static_assert` in earlier 
standards modes, diagnostics for the optional message were not reflected in C 
as they were in C++, etc.

This patch improves the diagnostic functionality in both C and C++ mode. It 
adds `-Wc89-c99-c11-c17-compat` and `-Wc89-c99-c11-c17-compat-pedantic` 
diagnostics groups and adds new C-specific diagnostics for a static assertion 
without a diagnostic message.


https://reviews.llvm.org/D95396

Files:
  clang/include/clang/Basic/DiagnosticGroups.td
  clang/include/clang/Basic/DiagnosticParseKinds.td
  clang/lib/Parse/ParseDeclCXX.cpp
  clang/test/Parser/static_assert.c
  clang/test/SemaCXX/static-assert.cpp

Index: clang/test/SemaCXX/static-assert.cpp
===
--- clang/test/SemaCXX/static-assert.cpp
+++ clang/test/SemaCXX/static-assert.cpp
@@ -1,4 +1,4 @@
-// RUN: %clang_cc1 -fsyntax-only -verify %s -std=c++11 -triple=x86_64-linux-gnu
+// RUN: %clang_cc1 -fsyntax-only -verify %s -std=c++11 -pedantic -triple=x86_64-linux-gnu
 
 int f(); // expected-note {{declared here}}
 
Index: clang/test/Parser/static_assert.c
===
--- /dev/null
+++ clang/test/Parser/static_assert.c
@@ -0,0 +1,45 @@
+// RUN: %clang_cc1 -fsyntax-only -std=c2x -DTEST_SPELLING -verify=c2x %s
+// RUN: %clang_cc1 -fsyntax-only -std=c2x -DTEST_SPELLING -fms-compatibility -verify=c2x-ms %s
+// RUN: %clang_cc1 -fsyntax-only -std=c2x -Wc89-c99-c11-c17-compat -verify=c2x-compat %s
+// RUN: %clang_cc1 -fsyntax-only -std=c99 -verify=c99 %s
+// RUN: %clang_cc1 -fsyntax-only -std=c99 -pedantic -verify=c99-pedantic %s
+// RUN: %clang_cc1 -fsyntax-only -std=c++17 -verify=cxx17 -x c++ %s
+// RUN: %clang_cc1 -fsyntax-only -std=c++17 -pedantic -verify=cxx17-pedantic -x c++ %s
+// RUN: %clang_cc1 -fsyntax-only -std=c++98 -verify=cxx98 -x c++ %s
+// RUN: %clang_cc1 -fsyntax-only -std=c++98 -pedantic -verify=cxx98-pedantic -x c++ %s
+// RUN: %clang_cc1 -fsyntax-only -std=c++17 -Wc++98-c++11-c++14-compat -verify=cxx17-compat -x c++ %s
+
+// c99-no-diagnostics
+// cxx17-no-diagnostics
+// cxx98-no-diagnostics
+
+#ifdef TEST_SPELLING
+// Only test the C++ spelling in C mode in some of the tests, to reduce the
+// amount of diagnostics to have to check. This spelling is allowed in MS-
+// compatibility mode in C, but otherwise produces errors.
+static_assert(1, ""); // c2x-error {{expected parameter declarator}} \
+  // c2x-error {{expected ')'}} \
+  // c2x-note {{to match this '('}} \
+  // c2x-warning {{type specifier missing, defaults to 'int'}} \
+  // c2x-ms-warning {{'static_assert' is spelled '_Static_assert' in C; consider including }}
+#endif
+
+// We support _Static_assert as an extension in older C modes and in all C++
+// modes, but only as a pedantic warning.
+_Static_assert(1, ""); // c99-pedantic-warning {{'_Static_assert' is a C11 extension}} \
+   // cxx17-pedantic-warning {{'_Static_assert' is a C11 extension}} \
+   // cxx98-pedantic-warning {{'_Static_assert' is a C11 extension}}
+
+// _Static_assert without a message has more complex diagnostic logic:
+//   * In C++17 or C2x mode, it's supported by default.
+//   * But there is a special compat warning flag to warn about portability to
+// older standards.
+//   * In older standard pedantic modes, warn about supporting without a
+// message as an extension.
+_Static_assert(1); // c99-pedantic-warning {{'_Static_assert' with no message is a C2x extension}} \
+   // cxx98-pedantic-warning {{'static_assert' with no message is a C++17 extension}} \
+   // c2x-compat-warning {{'_Static_assert' with no message is incompatible with C standards before C2x}} \
+   // cxx17-compat-warning {{'static_assert' with no message is incompatible with C++ standards before C++17}} \
+   // c99-pedantic-warning {{'_Static_assert' is a C11 extension}} \
+   // cxx17-pedantic-warning {{'_Static_assert' is a C11 extension}} \
+   // cxx98-pedantic-warning {{'_Static_assert' is a C11 extension}}
Index: clang/lib/Parse/ParseDeclCXX.cpp
===
--- clang/lib/Parse/ParseDeclCXX.cpp
+++ clang/lib/Parse/ParseDeclCXX.cpp
@@ -871,8 +871,13 @@
 
   if (Tok.is(tok::kw__Static_assert) && !getLangOpts().C11)
 Diag(Tok,