Index: docs/ReleaseNotes.rst =================================================================== --- docs/ReleaseNotes.rst +++ docs/ReleaseNotes.rst @@ -50,6 +50,58 @@ 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), including 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). + + .. 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 + + 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()) + ; + + 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. + } + Non-comprehensive list of changes in this release ------------------------------------------------- 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, + ExtraSemiStmt ]>; def Most : DiagGroup<"most", [ Index: include/clang/Basic/DiagnosticParseKinds.td =================================================================== --- include/clang/Basic/DiagnosticParseKinds.td +++ include/clang/Basic/DiagnosticParseKinds.td @@ -53,6 +53,9 @@ def warn_extra_semi_after_mem_fn_def : Warning< "extra ';' after member function definition">, InGroup, DefaultIgnore; +def warn_null_statement : Warning< + "';' with no preceding expression is a null statement">, + InGroup, DefaultIgnore; def ext_thread_before : Extension<"'__thread' before '%0'">; def ext_keyword_as_ident : ExtWarn< @@ -549,6 +552,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< + "init-statement of '%select{if|switch|range-based for}0' is a " + "null statement">, InGroup, DefaultIgnore; // C++ derived classes def err_dup_virtual : Error<"duplicate 'virtual' in base specifier">; 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 @@ -992,6 +992,15 @@ continue; } + if (Tok.is(tok::semi)) { + bool HasLeadingEmptyMacro = Tok.hasLeadingEmptyMacro(); + SourceLocation SemiLocation = Tok.getLocation(); + if (!HasLeadingEmptyMacro && !SemiLocation.isMacroID()) { + Diag(SemiLocation, diag::warn_null_statement) + << FixItHint::CreateRemoval(SemiLocation); + } + } + StmtResult R; if (Tok.isNot(tok::kw___extension__)) { R = ParseStatementOrDeclaration(Stmts, ACK_Any); @@ -1588,10 +1597,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 +1737,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 =================================================================== --- /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: test/Parser/extra-semi-resulting-in-nullstmt.cpp =================================================================== --- /dev/null +++ test/Parser/extra-semi-resulting-in-nullstmt.cpp @@ -0,0 +1,77 @@ +// RUN: %clang_cc1 -fsyntax-only -Wextra -verify %s +// 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 {{';' with no preceding expression is a null statement}} + ; // expected-warning {{';' with no preceding expression is a null statement}} + + // clang-format: off + ;; // expected-warning 2 {{';' with no preceding expression is a null statement}} + // clang-format: on + + {}; // expected-warning {{';' with no preceding expression is a null statement}} + + { + ; // expected-warning {{';' with no preceding expression is a null statement}} + } + + if (true) { + ; // expected-warning {{';' 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 {{';' with no preceding expression is a null statement}} + + BETTERMACRO(v5;); // expected-warning {{';' 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 + } +}