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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
