[cfe-dev] r347339 - [clang][Parse] Diagnose useless null statements / empty init-statements
George Karpenkov via cfe-dev
cfe-dev at lists.llvm.org
Tue Dec 4 17:04:09 PST 2018
> On Dec 4, 2018, at 4:47 PM, Aaron Ballman <aaron at aaronballman.com> wrote:
>
> On Tue, Dec 4, 2018 at 7:35 PM George Karpenkov <ekarpenkov at 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
This could work provided that all empty space preceding the semicolon is ignored (as the macro could be separated by space(s) or newline(s).
I’m not sure the complexity would be worth it, as I haven’t seen bugs arising from extra semicolons and empty statements,
but it would take care of my use case.
>
> ~Aaron
>
>>
>> Regards,
>> George
>>
>>
>>> On Nov 20, 2018, at 10:59 AM, Roman Lebedev via cfe-commits <cfe-commits at 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 at lists.llvm.org
>>> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
>>
More information about the cfe-dev
mailing list