Index: docs/ReleaseNotes.rst =================================================================== --- docs/ReleaseNotes.rst +++ docs/ReleaseNotes.rst @@ -55,6 +55,63 @@ 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 ------------------------------------------------- Index: include/clang/Basic/DiagnosticGroups.td =================================================================== --- include/clang/Basic/DiagnosticGroups.td +++ include/clang/Basic/DiagnosticGroups.td @@ -162,6 +162,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]>; @@ -768,7 +770,8 @@ MissingMethodReturnType, SignCompare, UnusedParameter, - NullPointerArithmetic + NullPointerArithmetic, + EmptyInitStatement ]>; def Most : DiagGroup<"most", [ 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, DefaultIgnore; +def warn_null_statement : Warning< + "empty expression statement has no effect; " + "remove unnecessary ';' to silence this warning">, + InGroup, DefaultIgnore; def ext_thread_before : Extension<"'__thread' before '%0'">; def ext_keyword_as_ident : ExtWarn< @@ -554,6 +558,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; +def warn_empty_init_statement : Warning< + "empty initialization statement of '%select{if|switch|range-based for}0' " + "has no effect">, InGroup, DefaultIgnore; // C++ derived classes def err_dup_virtual : Error<"duplicate 'virtual' in base specifier">; 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: 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: 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: test/Parser/extra-semi-resulting-in-nullstmt-in-init-statement.cpp =================================================================== --- test/Parser/extra-semi-resulting-in-nullstmt-in-init-statement.cpp +++ 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 {{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()) + ; +} Index: test/Parser/extra-semi-resulting-in-nullstmt.cpp =================================================================== --- test/Parser/extra-semi-resulting-in-nullstmt.cpp +++ test/Parser/extra-semi-resulting-in-nullstmt.cpp @@ -0,0 +1,96 @@ +// 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; + } +} + +[[]];