lebedev.ri updated this revision to Diff 168240.
lebedev.ri marked an inline comment as done.
lebedev.ri added a comment.
Thank you for taking a look!
- Reworded the diag as suggested a little
- Do consume consequtive semis as one
- Do still record each one of them in AST. I'm not sure if we want to do
that, but else existing tests break.
Repository:
rC Clang
https://reviews.llvm.org/D52695
Files:
docs/ReleaseNotes.rst
include/clang/Basic/DiagnosticGroups.td
include/clang/Basic/DiagnosticParseKinds.td
include/clang/Parse/Parser.h
lib/Parse/ParseExprCXX.cpp
lib/Parse/ParseStmt.cpp
test/Parser/extra-semi-resulting-in-nullstmt-in-init-statement.cpp
test/Parser/extra-semi-resulting-in-nullstmt.cpp
Index: test/Parser/extra-semi-resulting-in-nullstmt.cpp
===================================================================
--- /dev/null
+++ test/Parser/extra-semi-resulting-in-nullstmt.cpp
@@ -0,0 +1,81 @@
+// RUN: %clang_cc1 -fsyntax-only -Wextra-semi-stmt -verify %s
+// RUN: cp %s %t
+// RUN: %clang_cc1 -x c++ -Wextra-semi-stmt -fixit %t
+// RUN: %clang_cc1 -x c++ -Wextra-semi-stmt -Werror %t
+
+#define GOODMACRO(varname) int varname
+#define BETTERMACRO(varname) GOODMACRO(varname);
+#define NULLMACRO(varname)
+
+enum MyEnum {
+ E1,
+ E2
+};
+
+void test() {
+ ; // expected-warning {{statement has no effect, ';' with no preceding expression is a null statement}}
+ ;
+
+ // This removal of extra semi also consumes all the comments.
+ // clang-format: off
+ ;;;
+ // clang-format: on
+
+ // clang-format: off
+ ;NULLMACRO(ZZ);
+ // clang-format: on
+
+ {}; // expected-warning {{statement has no effect, ';' with no preceding expression is a null statement}}
+
+ {
+ ; // expected-warning {{statement has no effect, ';' with no preceding expression is a null statement}}
+ }
+
+ if (true) {
+ ; // expected-warning {{statement has no effect, ';' with no preceding expression is a null statement}}
+ }
+
+ GOODMACRO(v0); // OK
+
+ GOODMACRO(v1;) // OK
+
+ BETTERMACRO(v2) // OK
+
+ BETTERMACRO(v3;) // Extra ';', but within macro expansion, so ignored.
+
+ BETTERMACRO(v4); // expected-warning {{statement has no effect, ';' with no preceding expression is a null statement}}
+
+ BETTERMACRO(v5;); // expected-warning {{statement has no effect, ';' with no preceding expression is a null statement}}
+
+ NULLMACRO(v6) // OK
+
+ NULLMACRO(v7); // OK. This could be either GOODMACRO() or BETTERMACRO() situation, so we can't know we can drop it.
+
+ if (true)
+ ; // OK
+
+ while (true)
+ ; // OK
+
+ do
+ ; // OK
+ while (true);
+
+ for (;;) // OK
+ ; // OK
+
+ MyEnum my_enum;
+ switch (my_enum) {
+ case E1:
+ // stuff
+ break;
+ case E2:; // OK
+ }
+
+ for (;;) {
+ for (;;) {
+ goto contin_outer;
+ }
+ contin_outer:; // OK
+ }
+}
Index: test/Parser/extra-semi-resulting-in-nullstmt-in-init-statement.cpp
===================================================================
--- /dev/null
+++ test/Parser/extra-semi-resulting-in-nullstmt-in-init-statement.cpp
@@ -0,0 +1,64 @@
+// RUN: %clang_cc1 -fsyntax-only -Wextra -std=c++2a -verify %s
+// RUN: %clang_cc1 -fsyntax-only -Wextra-semi-stmt -std=c++2a -verify %s
+// RUN: %clang_cc1 -fsyntax-only -Wempty-init-stmt -std=c++2a -verify %s
+// RUN: cp %s %t
+// RUN: %clang_cc1 -x c++ -Wempty-init-stmt -std=c++2a -fixit %t
+// RUN: %clang_cc1 -x c++ -Wempty-init-stmt -std=c++2a -Werror %t
+
+struct S {
+ int *begin();
+ int *end();
+};
+
+void naive(int x) {
+ if (; true) // expected-warning {{init-statement of 'if' is a null statement}}
+ ;
+
+ switch (; x) { // expected-warning {{init-statement of 'switch' is a null statement}}
+ }
+
+ for (; int y : S()) // expected-warning {{init-statement of 'range-based for' is a null statement}}
+ ;
+
+ for (;;) // OK
+ ;
+}
+
+#define NULLMACRO
+
+void with_null_macro(int x) {
+ if (NULLMACRO; true)
+ ;
+
+ switch (NULLMACRO; x) {
+ }
+
+ for (NULLMACRO; int y : S())
+ ;
+}
+
+#define SEMIMACRO ;
+
+void with_semi_macro(int x) {
+ if (SEMIMACRO true)
+ ;
+
+ switch (SEMIMACRO x) {
+ }
+
+ for (SEMIMACRO int y : S())
+ ;
+}
+
+#define PASSTHROUGHMACRO(x) x
+
+void with_passthrough_macro(int x) {
+ if (PASSTHROUGHMACRO(;) true)
+ ;
+
+ switch (PASSTHROUGHMACRO(;) x) {
+ }
+
+ for (PASSTHROUGHMACRO(;) int y : S())
+ ;
+}
Index: lib/Parse/ParseStmt.cpp
===================================================================
--- lib/Parse/ParseStmt.cpp
+++ lib/Parse/ParseStmt.cpp
@@ -930,6 +930,34 @@
}
+/// Consume any extra semi-colons resulting in null statements,
+/// returning true if any tok::semi were consumed.
+bool Parser::ConsumeNullStmt(StmtVector &Stmts) {
+ if (!Tok.is(tok::semi))
+ return false;
+
+ SourceLocation StartLoc = Tok.getLocation();
+ SourceLocation EndLoc;
+
+ while (Tok.is(tok::semi) && !Tok.hasLeadingEmptyMacro() &&
+ Tok.getLocation().isValid() && !Tok.getLocation().isMacroID()) {
+ EndLoc = Tok.getLocation();
+
+ // Don't just ConsumeToken() this tok::semi, do store it in AST.
+ StmtResult R = ParseStatementOrDeclaration(Stmts, ACK_Any);
+ if (R.isUsable())
+ Stmts.push_back(R.get());
+ }
+
+ // Did not consume any extra semi.
+ if (EndLoc.isInvalid())
+ return false;
+
+ Diag(StartLoc, diag::warn_null_statement)
+ << FixItHint::CreateRemoval(SourceRange(StartLoc, EndLoc));
+ return true;
+}
+
/// ParseCompoundStatementBody - Parse a sequence of statements and invoke the
/// ActOnCompoundStmt action. This expects the '{' to be the current token, and
/// consume the '}' at the end of the block. It does not manipulate the scope
@@ -992,6 +1020,9 @@
continue;
}
+ if (ConsumeNullStmt(Stmts))
+ continue;
+
StmtResult R;
if (Tok.isNot(tok::kw___extension__)) {
R = ParseStatementOrDeclaration(Stmts, ACK_Any);
@@ -1588,10 +1619,15 @@
ParsedAttributesWithRange attrs(AttrFactory);
MaybeParseCXX11Attributes(attrs);
+ SourceLocation EmptyInitStmtSemiLoc;
+
// Parse the first part of the for specifier.
if (Tok.is(tok::semi)) { // for (;
ProhibitAttributes(attrs);
// no first part, eat the ';'.
+ SourceLocation SemiLoc = Tok.getLocation();
+ if (!Tok.hasLeadingEmptyMacro() && !SemiLoc.isMacroID())
+ EmptyInitStmtSemiLoc = SemiLoc;
ConsumeToken();
} else if (getLangOpts().CPlusPlus && Tok.is(tok::identifier) &&
isForRangeIdentifier()) {
@@ -1723,6 +1759,11 @@
: diag::ext_for_range_init_stmt)
<< (FirstPart.get() ? FirstPart.get()->getSourceRange()
: SourceRange());
+ if (EmptyInitStmtSemiLoc.isValid()) {
+ Diag(EmptyInitStmtSemiLoc, diag::warn_empty_init_statement)
+ << /*for-loop*/ 2
+ << FixItHint::CreateRemoval(EmptyInitStmtSemiLoc);
+ }
}
} else {
ExprResult SecondExpr = ParseExpression();
Index: lib/Parse/ParseExprCXX.cpp
===================================================================
--- lib/Parse/ParseExprCXX.cpp
+++ lib/Parse/ParseExprCXX.cpp
@@ -1773,7 +1773,13 @@
// if (; true);
if (InitStmt && Tok.is(tok::semi)) {
WarnOnInit();
- SourceLocation SemiLoc = ConsumeToken();
+ SourceLocation SemiLoc = Tok.getLocation();
+ if (!Tok.hasLeadingEmptyMacro() && !SemiLoc.isMacroID()) {
+ Diag(SemiLoc, diag::warn_empty_init_statement)
+ << (CK == Sema::ConditionKind::Switch)
+ << FixItHint::CreateRemoval(SemiLoc);
+ }
+ ConsumeToken();
*InitStmt = Actions.ActOnNullStmt(SemiLoc);
return ParseCXXCondition(nullptr, Loc, CK);
}
Index: include/clang/Parse/Parser.h
===================================================================
--- include/clang/Parse/Parser.h
+++ include/clang/Parse/Parser.h
@@ -1888,6 +1888,7 @@
StmtResult ParseCompoundStatement(bool isStmtExpr,
unsigned ScopeFlags);
void ParseCompoundStatementLeadingPragmas();
+ bool ConsumeNullStmt(StmtVector &Stmts);
StmtResult ParseCompoundStatementBody(bool isStmtExpr = false);
bool ParseParenExprOrCondition(StmtResult *InitStmt,
Sema::ConditionResult &CondResult,
Index: include/clang/Basic/DiagnosticParseKinds.td
===================================================================
--- include/clang/Basic/DiagnosticParseKinds.td
+++ include/clang/Basic/DiagnosticParseKinds.td
@@ -53,6 +53,10 @@
def warn_extra_semi_after_mem_fn_def : Warning<
"extra ';' after member function definition">,
InGroup<ExtraSemi>, DefaultIgnore;
+def warn_null_statement : Warning<
+ "statement has no effect, ';' "
+ "with no preceding expression is a null statement">,
+ InGroup<ExtraSemiStmt>, DefaultIgnore;
def ext_thread_before : Extension<"'__thread' before '%0'">;
def ext_keyword_as_ident : ExtWarn<
@@ -549,6 +553,9 @@
def warn_cxx17_compat_for_range_init_stmt : Warning<
"range-based for loop initialization statements are incompatible with "
"C++ standards before C++2a">, DefaultIgnore, InGroup<CXXPre2aCompat>;
+def warn_empty_init_statement : Warning<
+ "init-statement of '%select{if|switch|range-based for}0' is a "
+ "null statement">, InGroup<EmptyInitStatement>, DefaultIgnore;
// C++ derived classes
def err_dup_virtual : Error<"duplicate 'virtual' in base specifier">;
Index: include/clang/Basic/DiagnosticGroups.td
===================================================================
--- include/clang/Basic/DiagnosticGroups.td
+++ include/clang/Basic/DiagnosticGroups.td
@@ -160,6 +160,8 @@
def ExtraTokens : DiagGroup<"extra-tokens">;
def CXX98CompatExtraSemi : DiagGroup<"c++98-compat-extra-semi">;
def CXX11ExtraSemi : DiagGroup<"c++11-extra-semi">;
+def EmptyInitStatement : DiagGroup<"empty-init-stmt">;
+def ExtraSemiStmt : DiagGroup<"extra-semi-stmt", [EmptyInitStatement]>;
def ExtraSemi : DiagGroup<"extra-semi", [CXX98CompatExtraSemi,
CXX11ExtraSemi]>;
@@ -764,7 +766,8 @@
MissingMethodReturnType,
SignCompare,
UnusedParameter,
- NullPointerArithmetic
+ NullPointerArithmetic,
+ EmptyInitStatement
]>;
def Most : DiagGroup<"most", [
Index: docs/ReleaseNotes.rst
===================================================================
--- docs/ReleaseNotes.rst
+++ docs/ReleaseNotes.rst
@@ -50,6 +50,72 @@
Improvements to Clang's diagnostics
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+- ``-Wextra-semi-stmt`` is a new diagnostic, which is, much like
+ ``-Wextra-semi``, diagnoses extra semicolons. This new diagnostic
+ fires on all *unnecessary* null statements (expression statements without
+ an expression), unless: the semi directly follows a macro that was
+ expanded to nothing; the semi is within the macro itself (both macros from
+ system headers, and normal macros).
+
+ .. code-block:: c++
+
+ #define MACRO(x) int x;
+ #define NULLMACRO(varname)
+
+ struct S {
+ int *begin();
+ int *end();
+ };
+
+ void test() {
+ ; // <- warning: ';' with no preceding expression is a null statement
+
+ while (true)
+ ; // OK, it is needed.
+
+ switch (my_enum) {
+ case E1:
+ // stuff
+ break;
+ case E2:
+ ; // OK, it is needed.
+ }
+
+ MACRO(v0;) // Extra semi, but within macro, so ignored.
+
+ MACRO(v1); // <- warning: ';' with no preceding expression is a null statement
+
+ NULLMACRO(v2); // ignored, NULLMACRO expanded to nothing.
+ }
+
+- ``-Wempty-init-stmt`` is a new diagnostic, which is diagnoses empty
+ init-statements of ``if``, ``switch``, ``range-based for``, unless: the semi
+ directly follows a macro that was expanded to nothing; the semi is within the
+ macro itself (both macros from system headers, and normal macros).
+ This diagnostic in ``-Wextra-semi-stmt`` group, and is enabled in ``-Wextra``.
+
+ .. code-block:: c++
+
+ struct S {
+ int *begin();
+ int *end();
+ };
+
+ void test() {
+ if(; // <- warning: init-statement of 'if' is a null statement
+ true)
+ ;
+
+ switch (; // <- warning: init-statement of 'switch' is a null statement
+ x) {
+ ...
+ }
+
+ for (; // <- warning: init-statement of 'range-based for' is a null statement
+ int y : S())
+ ;
+ }
+
Non-comprehensive list of changes in this release
-------------------------------------------------
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
