On Tue, Dec 4, 2018 at 7:35 PM George Karpenkov <ekarpen...@apple.com> wrote: > > Hi Roman, > > I’m against this new warning. > > A very common idiom is defining a “DEBUG” macro to be a no-op in release, and > a logging function otherwise. > The new introduced warning warns on all such cases (e.g. 'DEBUG(“mystring”);')
Yeah, that's not good. > As noted in the review, Clang warnings should generally not be used to > diagnose style issues — and an extra semicolon does not seem to be a common > bug source. > > It is a problem in practice for us since we have projects which enable all > available warnings and also enable “-Werror”. Would you be okay with the warning if it was only diagnosed when the source location of the semi-colon is not immediately preceded by a macro expansion location? e.g., EMPTY_EXPANSION(12); // Not diagnosed EMPTY_EXPANSION; // Not diagnosed ; // Diagnosed ~Aaron > > Regards, > George > > > > On Nov 20, 2018, at 10:59 AM, Roman Lebedev via cfe-commits > > <cfe-commits@lists.llvm.org> wrote: > > > > Author: lebedevri > > Date: Tue Nov 20 10:59:05 2018 > > New Revision: 347339 > > > > URL: http://llvm.org/viewvc/llvm-project?rev=347339&view=rev > > Log: > > [clang][Parse] Diagnose useless null statements / empty init-statements > > > > 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 > > > > Added: > > > > cfe/trunk/test/Parser/extra-semi-resulting-in-nullstmt-in-init-statement.cpp > > cfe/trunk/test/Parser/extra-semi-resulting-in-nullstmt.cpp > > Modified: > > cfe/trunk/docs/ReleaseNotes.rst > > cfe/trunk/include/clang/Basic/DiagnosticGroups.td > > cfe/trunk/include/clang/Basic/DiagnosticParseKinds.td > > cfe/trunk/include/clang/Parse/Parser.h > > cfe/trunk/lib/Parse/ParseExprCXX.cpp > > cfe/trunk/lib/Parse/ParseStmt.cpp > > > > Modified: cfe/trunk/docs/ReleaseNotes.rst > > URL: > > http://llvm.org/viewvc/llvm-project/cfe/trunk/docs/ReleaseNotes.rst?rev=347339&r1=347338&r2=347339&view=diff > > ============================================================================== > > --- cfe/trunk/docs/ReleaseNotes.rst (original) > > +++ cfe/trunk/docs/ReleaseNotes.rst Tue Nov 20 10:59:05 2018 > > @@ -55,6 +55,63 @@ Major New Features > > 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 > > ------------------------------------------------- > > > > Modified: cfe/trunk/include/clang/Basic/DiagnosticGroups.td > > URL: > > http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Basic/DiagnosticGroups.td?rev=347339&r1=347338&r2=347339&view=diff > > ============================================================================== > > --- cfe/trunk/include/clang/Basic/DiagnosticGroups.td (original) > > +++ cfe/trunk/include/clang/Basic/DiagnosticGroups.td Tue Nov 20 10:59:05 > > 2018 > > @@ -162,6 +162,8 @@ def GNUEmptyStruct : DiagGroup<"gnu-empt > > 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 @@ def Extra : DiagGroup<"extra", [ > > MissingMethodReturnType, > > SignCompare, > > UnusedParameter, > > - NullPointerArithmetic > > + NullPointerArithmetic, > > + EmptyInitStatement > > ]>; > > > > def Most : DiagGroup<"most", [ > > > > Modified: cfe/trunk/include/clang/Basic/DiagnosticParseKinds.td > > URL: > > http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Basic/DiagnosticParseKinds.td?rev=347339&r1=347338&r2=347339&view=diff > > ============================================================================== > > --- cfe/trunk/include/clang/Basic/DiagnosticParseKinds.td (original) > > +++ cfe/trunk/include/clang/Basic/DiagnosticParseKinds.td Tue Nov 20 > > 10:59:05 2018 > > @@ -53,6 +53,10 @@ def ext_extra_semi_cxx11 : Extension< > > 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< > > @@ -554,6 +558,9 @@ def ext_for_range_init_stmt : 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">; > > > > Modified: cfe/trunk/include/clang/Parse/Parser.h > > URL: > > http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Parse/Parser.h?rev=347339&r1=347338&r2=347339&view=diff > > ============================================================================== > > --- cfe/trunk/include/clang/Parse/Parser.h (original) > > +++ cfe/trunk/include/clang/Parse/Parser.h Tue Nov 20 10:59:05 2018 > > @@ -1888,6 +1888,7 @@ private: > > StmtResult ParseCompoundStatement(bool isStmtExpr, > > unsigned ScopeFlags); > > void ParseCompoundStatementLeadingPragmas(); > > + bool ConsumeNullStmt(StmtVector &Stmts); > > StmtResult ParseCompoundStatementBody(bool isStmtExpr = false); > > bool ParseParenExprOrCondition(StmtResult *InitStmt, > > Sema::ConditionResult &CondResult, > > > > Modified: cfe/trunk/lib/Parse/ParseExprCXX.cpp > > URL: > > http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Parse/ParseExprCXX.cpp?rev=347339&r1=347338&r2=347339&view=diff > > ============================================================================== > > --- cfe/trunk/lib/Parse/ParseExprCXX.cpp (original) > > +++ cfe/trunk/lib/Parse/ParseExprCXX.cpp Tue Nov 20 10:59:05 2018 > > @@ -1773,7 +1773,13 @@ Sema::ConditionResult Parser::ParseCXXCo > > // 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); > > } > > > > Modified: cfe/trunk/lib/Parse/ParseStmt.cpp > > URL: > > http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Parse/ParseStmt.cpp?rev=347339&r1=347338&r2=347339&view=diff > > ============================================================================== > > --- cfe/trunk/lib/Parse/ParseStmt.cpp (original) > > +++ cfe/trunk/lib/Parse/ParseStmt.cpp Tue Nov 20 10:59:05 2018 > > @@ -930,6 +930,34 @@ void Parser::ParseCompoundStatementLeadi > > > > } > > > > +/// 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 @@ StmtResult Parser::ParseCompoundStatemen > > continue; > > } > > > > + if (ConsumeNullStmt(Stmts)) > > + continue; > > + > > StmtResult R; > > if (Tok.isNot(tok::kw___extension__)) { > > R = ParseStatementOrDeclaration(Stmts, ACK_Any); > > @@ -1588,10 +1619,15 @@ StmtResult Parser::ParseForStatement(Sou > > 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 @@ StmtResult Parser::ParseForStatement(Sou > > : 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(); > > > > Added: > > cfe/trunk/test/Parser/extra-semi-resulting-in-nullstmt-in-init-statement.cpp > > URL: > > http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Parser/extra-semi-resulting-in-nullstmt-in-init-statement.cpp?rev=347339&view=auto > > ============================================================================== > > --- > > cfe/trunk/test/Parser/extra-semi-resulting-in-nullstmt-in-init-statement.cpp > > (added) > > +++ > > cfe/trunk/test/Parser/extra-semi-resulting-in-nullstmt-in-init-statement.cpp > > Tue Nov 20 10:59:05 2018 > > @@ -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()) > > + ; > > +} > > > > Added: cfe/trunk/test/Parser/extra-semi-resulting-in-nullstmt.cpp > > URL: > > http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Parser/extra-semi-resulting-in-nullstmt.cpp?rev=347339&view=auto > > ============================================================================== > > --- cfe/trunk/test/Parser/extra-semi-resulting-in-nullstmt.cpp (added) > > +++ cfe/trunk/test/Parser/extra-semi-resulting-in-nullstmt.cpp Tue Nov 20 > > 10:59:05 2018 > > @@ -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; > > + } > > +} > > + > > +[[]]; > > > > > > _______________________________________________ > > cfe-commits mailing list > > cfe-commits@lists.llvm.org > > http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits > _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
