On Fri, Dec 28, 2018 at 2:57 AM Nico Weber <tha...@chromium.org> wrote:
>
> Should this be in -Wextra-semi? It seems weird to have to pass both
> -Wextra-semi and -Wextra-semi-stmt.
It does seem somewhat weird, and my initial implementation did add it
into -Wextra-semi,
but the review https://reviews.llvm.org/D52695#inline-464922 said to
keep them separate.
> (Also, we generally try to not add DefaultIgnore warnings. I think this is a
> useful warning though, and other than others in this thread I think it'd be
> good to have -Wextra-semi in -Wall. But I don't feel strongly about that
> part.)
By -Wextra-semi here you mean -Wextra-semi + -Wextra-semi-stmt ?
Note that even -Wempty-init-stmt is only in -Wextra. I guess that one
can be realistically upgraded into -Wall.
-Wextra-semi{,-stmt} are less clear-cut. As we have seen, even having
-Wextra-semi-stmt in -Weverything
results in negative feedback. It might be possible to uplift them both
into -Wextra, but realistically i don't think
it will be possible to handle the feedback of making them part of -Wall.
(also, maybe "C++98 requires newline at end of file
[-Wc++98-compat-pedantic]" should be separate, and in -Wall, too.)
Roman.
> On Tue, Nov 20, 2018 at 2:01 PM 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
