Summary:
clang has `-Wextra-semi` (D43162), which is not dictated by the currently selected standard.
While that is great, there is at least one more source of need-less semis - 'null statements'.
Sometimes, they are needed:
```
for(int x = 0; continueToDoWork(x); x++)
; // Ugly code, but the semi is needed here.
```
But sometimes they are just there for no reason:
```
switch(X) {
case 0:
return -2345;
case 5:
return 0;
default:
return 42;
}; // <- oops
;;;;;;;;;;; <- OOOOPS, still not diagnosed. Clearly this is junk.
```
Additionally:
```
if(; // <- empty init-statement
true)
;
switch (; // empty init-statement
x) {
...
}
for (; // <- empty init-statement
int y : S())
;
}
As usual, things may or may not go sideways in the presence of macros.
While evaluating this diag on my codebase of interest, it was unsurprisingly
discovered that Google Test macros are *very* prone to this.
And it seems many issues are deep within the GTest itself, not
in the snippets passed from the codebase that uses GTest.
So after some thought, i decided not do issue a diagnostic if the semi
is within *any* macro, be it either from the normal header, or system header.
Fixes [[ https://bugs.llvm.org/show_bug.cgi?id=39111 | PR39111 ]]
Reviewers: rsmith, aaron.ballman, efriedma
Reviewed By: aaron.ballman
Subscribers: cfe-commits
Differential Revision: https://reviews.llvm.org/D52695
git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@347339
91177308-0d34-0410-b5e6-
96231b3b80d8
Improvements to Clang's diagnostics
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+- ``-Wextra-semi-stmt`` is a new diagnostic that diagnoses extra semicolons,
+ much like ``-Wextra-semi``. This new diagnostic diagnoses all *unnecessary*
+ null statements (expression statements without an expression), unless: the
+ semicolon directly follows a macro that was expanded to nothing or if the
+ semicolon is within the macro itself. This applies to macros defined in system
+ headers as well as user-defined macros.
+
+ .. code-block:: c++
+
+ #define MACRO(x) int x;
+ #define NULLMACRO(varname)
+
+ 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 semicolon, 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 that diagnoses empty init-statements
+ of ``if``, ``switch``, ``range-based for``, unless: the semicolon directly
+ follows a macro that was expanded to nothing or if the semicolon is within the
+ macro itself (both macros from system headers, and normal macros). This
+ diagnostic is in the ``-Wextra-semi-stmt`` group and is enabled in
+ ``-Wextra``.
+
+ .. code-block:: c++
+
+ 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
-------------------------------------------------
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]>;
MissingMethodReturnType,
SignCompare,
UnusedParameter,
- NullPointerArithmetic
+ NullPointerArithmetic,
+ EmptyInitStatement
]>;
def Most : DiagGroup<"most", [
def warn_extra_semi_after_mem_fn_def : Warning<
"extra ';' after member function definition">,
InGroup<ExtraSemi>, DefaultIgnore;
+def warn_null_statement : Warning<
+ "empty expression statement has no effect; "
+ "remove unnecessary ';' to silence this warning">,
+ InGroup<ExtraSemiStmt>, DefaultIgnore;
def ext_thread_before : Extension<"'__thread' before '%0'">;
def ext_keyword_as_ident : ExtWarn<
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<
+ "empty initialization statement of '%select{if|switch|range-based for}0' "
+ "has no effect">, InGroup<EmptyInitStatement>, DefaultIgnore;
// C++ derived classes
def err_dup_virtual : Error<"duplicate 'virtual' in base specifier">;
StmtResult ParseCompoundStatement(bool isStmtExpr,
unsigned ScopeFlags);
void ParseCompoundStatementLeadingPragmas();
+ bool ConsumeNullStmt(StmtVector &Stmts);
StmtResult ParseCompoundStatementBody(bool isStmtExpr = false);
bool ParseParenExprOrCondition(StmtResult *InitStmt,
Sema::ConditionResult &CondResult,
// 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);
}
}
+/// 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
continue;
}
+ if (ConsumeNullStmt(Stmts))
+ continue;
+
StmtResult R;
if (Tok.isNot(tok::kw___extension__)) {
R = ParseStatementOrDeclaration(Stmts, ACK_Any);
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()) {
: 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();
--- /dev/null
+// 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 {{empty initialization statement of 'if' has no effect}}
+ ;
+
+ switch (; x) { // expected-warning {{empty initialization statement of 'switch' has no effect}}
+ }
+
+ for (; int y : S()) // expected-warning {{empty initialization statement of 'range-based for' has no effect}}
+ ;
+
+ 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())
+ ;
+}
--- /dev/null
+// 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 {{empty expression statement has no effect; remove unnecessary ';' to silence this warning}}
+ ;
+
+ // 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 {{empty expression statement has no effect; remove unnecessary ';' to silence this warning}}
+
+ {
+ ; // expected-warning {{empty expression statement has no effect; remove unnecessary ';' to silence this warning}}
+ }
+
+ if (true) {
+ ; // expected-warning {{empty expression statement has no effect; remove unnecessary ';' to silence this warning}}
+ }
+
+ GOODMACRO(v0); // OK
+
+ GOODMACRO(v1;) // OK
+
+ BETTERMACRO(v2) // OK
+
+ BETTERMACRO(v3;) // Extra ';', but within macro expansion, so ignored.
+
+ BETTERMACRO(v4); // expected-warning {{empty expression statement has no effect; remove unnecessary ';' to silence this warning}}
+
+ BETTERMACRO(v5;); // expected-warning {{empty expression statement has no effect; remove unnecessary ';' to silence this warning}}
+
+ 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
+ }
+}
+
+;
+
+namespace NS {};
+
+void foo(int x) {
+ switch (x) {
+ case 0:
+ [[fallthrough]];
+ case 1:
+ return;
+ }
+}
+
+[[]];
projects / clang / commitdiff