[PATCH] D129048: Rewording the "static_assert" to static assertion

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

I think this is getting pretty close! I did have some style guideline changes, and at least one test addition I'd like to see though. Also, it looks like precommit CI caught a test that still needs to be updated:

  FAIL: Clang :: SemaCXX/cxx98-compat.cpp (16448 of 68726)
  
  ******************** TEST 'Clang :: SemaCXX/cxx98-compat.cpp' FAILED ********************
  
  Script:
  
  --
  
  : 'RUN: at line 1';   /var/lib/buildkite-agent/builds/llvm-project/build/bin/clang -cc1 -internal-isystem /var/lib/buildkite-agent/builds/llvm-project/build/lib/clang/15.0.0/include -nostdsysteminc -fsyntax-only -std=c++11 -Wc++98-compat -verify /var/lib/buildkite-agent/builds/llvm-project/clang/test/SemaCXX/cxx98-compat.cpp
  
  : 'RUN: at line 2';   /var/lib/buildkite-agent/builds/llvm-project/build/bin/clang -cc1 -internal-isystem /var/lib/buildkite-agent/builds/llvm-project/build/lib/clang/15.0.0/include -nostdsysteminc -fsyntax-only -std=c++14 -Wc++98-compat -verify /var/lib/buildkite-agent/builds/llvm-project/clang/test/SemaCXX/cxx98-compat.cpp -DCXX14COMPAT
  
  : 'RUN: at line 3';   /var/lib/buildkite-agent/builds/llvm-project/build/bin/clang -cc1 -internal-isystem /var/lib/buildkite-agent/builds/llvm-project/build/lib/clang/15.0.0/include -nostdsysteminc -fsyntax-only -std=c++17 -Wc++98-compat -verify /var/lib/buildkite-agent/builds/llvm-project/clang/test/SemaCXX/cxx98-compat.cpp -DCXX14COMPAT -DCXX17COMPAT
  
  --
  
  Exit Code: 1
  
  
  
  Command Output (stderr):
  
  --
  
  error: 'warning' diagnostics expected but not seen: 
  
    File /var/lib/buildkite-agent/builds/llvm-project/clang/test/SemaCXX/cxx98-compat.cpp Line 155: static_assert declarations are incompatible with C++98
  
  error: 'warning' diagnostics seen but not expected: 
  
    File /var/lib/buildkite-agent/builds/llvm-project/clang/test/SemaCXX/cxx98-compat.cpp Line 155: 'static_assert' declarations are incompatible with C++98
  
  2 errors generated.
  
  
  
  --
  
  
  
  ********************



================
Comment at: clang/include/clang/Basic/DiagnosticParseKinds.td:285
 def err_expected_semi_after_static_assert : Error<
-  "expected ';' after static_assert">;
+  "expected ';' after '%0'">;
 def err_expected_semi_for : Error<"expected ';' in 'for' statement specifier">;
----------------
I think we're missing some test coverage, because I would have expected at least one test to fail because of this change. I think you should add a test to `clang/test/SemaCXX/static-assert.cpp` like this:

```
static_assert(1, "")
_Static_assert(1, "")
```
where the semi colons are missing, and add the correct `expected-error` lines to each so that we have coverage for your changes.


================
Comment at: clang/include/clang/Parse/Parser.h:1046
   /// to the semicolon, consumes that extra token.
-  bool ExpectAndConsumeSemi(unsigned DiagID);
+  bool ExpectAndConsumeSemi(unsigned DiagID , StringRef tokenUsed = "");
 
----------------



================
Comment at: clang/lib/Parse/ParseDeclCXX.cpp:930
+  // saving the token used for static assertion
+  Token savedTok = Tok;
+
----------------



================
Comment at: clang/lib/Parse/ParseDeclCXX.cpp:994-998
+  // saving the token used the syntax of the static assertion
+  const char * tokenUsed = savedTok.getName();
+
   DeclEnd = Tok.getLocation();
+  ExpectAndConsumeSemi(diag::err_expected_semi_after_static_assert , tokenUsed); //passing the token used to the error message
----------------
You should also make sure this fits in the usual 80-col limit and if it doesn't, clang-format the patch (https://clang.llvm.org/docs/ClangFormat.html#script-for-patch-reformatting).


================
Comment at: clang/lib/Parse/Parser.cpp:156-175
+bool Parser::ExpectAndConsumeSemi(unsigned DiagID, StringRef tokenUsed) {
   if (TryConsumeToken(tok::semi))
     return false;
 
   if (Tok.is(tok::code_completion)) {
     handleUnexpectedCodeCompletionToken();
     return false;
----------------



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D129048