Re: r347339 - [clang][Parse] Diagnose useless null statements / empty init-statements

classic Classic list List threaded Threaded
12 messages Options
Reply | Threaded
Open this post in threaded view
|

Re: r347339 - [clang][Parse] Diagnose useless null statements / empty init-statements

Oleg Smolsky via cfe-dev
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”);')

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”.

Regards,
George


> On Nov 20, 2018, at 10:59 AM, Roman Lebedev via cfe-commits <[hidden email]> 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
> [hidden email]
> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: r347339 - [clang][Parse] Diagnose useless null statements / empty init-statements

Oleg Smolsky via cfe-dev
On Tue, Dec 4, 2018 at 7:35 PM George Karpenkov <[hidden email]> 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 <[hidden email]> 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
> > [hidden email]
> > http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
>
_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: r347339 - [clang][Parse] Diagnose useless null statements / empty init-statements

Oleg Smolsky via cfe-dev


> On Dec 4, 2018, at 4:47 PM, Aaron Ballman <[hidden email]> wrote:
>
> On Tue, Dec 4, 2018 at 7:35 PM George Karpenkov <[hidden email]> 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 <[hidden email]> 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
>>> [hidden email]
>>> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
>>

_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: r347339 - [clang][Parse] Diagnose useless null statements / empty init-statements

Oleg Smolsky via cfe-dev


On 12/4/18 5:04 PM, George Karpenkov via cfe-dev wrote:

>
>> On Dec 4, 2018, at 4:47 PM, Aaron Ballman <[hidden email]> wrote:
>>
>> On Tue, Dec 4, 2018 at 7:35 PM George Karpenkov <[hidden email]> 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.
>

Or rather when the *previous* semicolon is coming from a macro.

>> ~Aaron
>>
>>> Regards,
>>> George
>>>
>>>
>>>> On Nov 20, 2018, at 10:59 AM, Roman Lebedev via cfe-commits <[hidden email]> 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
>>>> [hidden email]
>>>> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
> _______________________________________________
> cfe-dev mailing list
> [hidden email]
> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev

_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: r347339 - [clang][Parse] Diagnose useless null statements / empty init-statements

Oleg Smolsky via cfe-dev
On Wed, Dec 5, 2018 at 4:07 AM Artem Dergachev via cfe-commits
<[hidden email]> wrote:
>
>
>
> On 12/4/18 5:04 PM, George Karpenkov via cfe-dev wrote:
> >
> >> On Dec 4, 2018, at 4:47 PM, Aaron Ballman <[hidden email]> wrote:
> >>
> >> On Tue, Dec 4, 2018 at 7:35 PM George Karpenkov <[hidden email]> wrote:
> >>> Hi Roman,
Hi.

> >>> 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.
It does warn on such cases when the macro not expanded to nothing, yes.

> >>> 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.
I don't disagree.
The alternative solution would be to try to squeeze this bit of info
into the AST,
without increasing the size of the particular type, and then emit the
diag in clang-tidy.
I did consider it, it seemed like a over-engineering.

> >>> It is a problem in practice for us since we have projects which enable all available warnings and also enable “-Werror”.
Hm, they have asked for -Weverything and they got it. Seems to be
working as intended.
When in previous discussions elsewhere i have talked about
-Weverything, i was always
been told that it isn't supposed to be used like that (admittedly, i
*do* use it like that :)).

Could you explain how is this different from any other warning that is
considered pointless by the project?
Why not simply disable it?

> >> 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.
> >
>
> Or rather when the *previous* semicolon is coming from a macro.
I don't like this. clang is already wa-a-ay too forgiving about
macros, it almost ignores almost every warning in macros.
It was an *intentional* decision to still warn on useless ';' after
non-empty macros.
So at most i would be ok with emitting the macro case under -W{flag}-macro.
Thoughts?

> >> ~Aaron
> >>
> >>> Regards,
> >>> George
Roman.

> >>>> On Nov 20, 2018, at 10:59 AM, Roman Lebedev via cfe-commits <[hidden email]> 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
> >>>> [hidden email]
> >>>> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
> > _______________________________________________
> > cfe-dev mailing list
> > [hidden email]
> > http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
>
> _______________________________________________
> cfe-commits mailing list
> [hidden email]
> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: r347339 - [clang][Parse] Diagnose useless null statements / empty init-statements

Oleg Smolsky via cfe-dev
On Wed, Dec 5, 2018 at 1:40 AM Roman Lebedev <[hidden email]> wrote:

>
> On Wed, Dec 5, 2018 at 4:07 AM Artem Dergachev via cfe-commits
> <[hidden email]> wrote:
> >
> >
> >
> > On 12/4/18 5:04 PM, George Karpenkov via cfe-dev wrote:
> > >
> > >> On Dec 4, 2018, at 4:47 PM, Aaron Ballman <[hidden email]> wrote:
> > >>
> > >> On Tue, Dec 4, 2018 at 7:35 PM George Karpenkov <[hidden email]> wrote:
> > >>> Hi Roman,
> Hi.
>
> > >>> 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.
> It does warn on such cases when the macro not expanded to nothing, yes.
>
> > >>> 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.
> I don't disagree.
> The alternative solution would be to try to squeeze this bit of info
> into the AST,
> without increasing the size of the particular type, and then emit the
> diag in clang-tidy.
> I did consider it, it seemed like a over-engineering.
>
> > >>> It is a problem in practice for us since we have projects which enable all available warnings and also enable “-Werror”.
> Hm, they have asked for -Weverything and they got it. Seems to be
> working as intended.
> When in previous discussions elsewhere i have talked about
> -Weverything, i was always
> been told that it isn't supposed to be used like that (admittedly, i
> *do* use it like that :)).
>
> Could you explain how is this different from any other warning that is
> considered pointless by the project?
> Why not simply disable it?
>
> > >> 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.
> > >
> >
> > Or rather when the *previous* semicolon is coming from a macro.
> I don't like this. clang is already wa-a-ay too forgiving about
> macros, it almost ignores almost every warning in macros.
> It was an *intentional* decision to still warn on useless ';' after
> non-empty macros.

I think I'm confused now. This is a test case:

NULLMACRO(v7); // OK. This could be either GOODMACRO() or
BETTERMACRO() situation, so we can't know we can drop it.

So empty macro expansions should be ignored as I expected... so what
is being diagnosed? George, can you provide a more clear example that
demonstrates the issue?

To be clear, I do *not* think this is a situation we should spend
effort supporting (assuming "all available warnings" really means
-Weverything and not something else):

> It is a problem in practice for us since we have projects which enable all available warnings and also enable “-Werror”.

However, if we are diagnosing *empty* macro expansions followed by a
semi-colon, I think that would be unacceptably chatty and is worth
fixing on its own merits.

~Aaron

>
> > >> ~Aaron
> > >>
> > >>> Regards,
> > >>> George
> Roman.
>
> > >>>> On Nov 20, 2018, at 10:59 AM, Roman Lebedev via cfe-commits <[hidden email]> 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
> > >>>> [hidden email]
> > >>>> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
> > > _______________________________________________
> > > cfe-dev mailing list
> > > [hidden email]
> > > http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
> >
> > _______________________________________________
> > cfe-commits mailing list
> > [hidden email]
> > http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: r347339 - [clang][Parse] Diagnose useless null statements / empty init-statements

Oleg Smolsky via cfe-dev
These are the cases I get:

#define unexpected(a) { kprintf("unexpected %s:%d\n", __FILE__, __LINE__); a; }

and

#if 0
#define DEBG(fmt, args...)      { IOLog(fmt, ## args); kprintf(fmt, ## args); }
#else
#define DEBG(fmt, args...)      {}
#endif

Now calls

`DEBG(‘x’);` and `unexpected(‘msg’);`

cause the warning to be fired.
Of course, semicolon could be omitted, but the fact that the macro has braces is sort of an implementation detail.

Greg Parker has pointed out it’s possible to rewrite the macros using the “do { … } while (0)”,
but that is a lot of macros to change, and the resulting code would be arguably less readable.

On Dec 5, 2018, at 5:10 AM, Aaron Ballman <[hidden email]> wrote:

On Wed, Dec 5, 2018 at 1:40 AM Roman Lebedev <[hidden email]> wrote:

It is a problem in practice for us since we have projects which enable all available warnings and also enable “-Werror”.
Hm, they have asked for -Weverything and they got it. Seems to be
working as intended.
When in previous discussions elsewhere i have talked about
-Weverything, i was always
been told that it isn't supposed to be used like that (admittedly, i
*do* use it like that :)).

Could you explain how is this different from any other warning that is
considered pointless by the project?
Why not simply disable it?

You are right, it could be disabled. It’s just for this warning the noise vs. usefulness ratio seemed pretty low.


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.


Or rather when the *previous* semicolon is coming from a macro.
I don't like this. clang is already wa-a-ay too forgiving about
macros, it almost ignores almost every warning in macros.
It was an *intentional* decision to still warn on useless ';' after
non-empty macros.

I think I'm confused now. This is a test case:

NULLMACRO(v7); // OK. This could be either GOODMACRO() or
BETTERMACRO() situation, so we can't know we can drop it.

So empty macro expansions should be ignored as I expected... so what
is being diagnosed? George, can you provide a more clear example that
demonstrates the issue?

To be clear, I do *not* think this is a situation we should spend
effort supporting (assuming "all available warnings" really means
-Weverything and not something else):

It is a problem in practice for us since we have projects which enable all available warnings and also enable “-Werror”.

However, if we are diagnosing *empty* macro expansions followed by a
semi-colon, I think that would be unacceptably chatty and is worth
fixing on its own merits.

~Aaron


~Aaron

Regards,
George
Roman.

On Nov 20, 2018, at 10:59 AM, Roman Lebedev via cfe-commits <[hidden email]> 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
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev

_______________________________________________
cfe-commits mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits


_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: r347339 - [clang][Parse] Diagnose useless null statements / empty init-statements

Oleg Smolsky via cfe-dev
On Wed, 5 Dec 2018, 10:01 George Karpenkov via cfe-commits <[hidden email] wrote:
These are the cases I get:

#define unexpected(a) { kprintf("unexpected %s:%d\n", __FILE__, __LINE__); a; }

and

#if 0
#define DEBG(fmt, args...)      { IOLog(fmt, ## args); kprintf(fmt, ## args); }
#else
#define DEBG(fmt, args...)      {}
#endif

Now calls

`DEBG(‘x’);` and `unexpected(‘msg’);`

cause the warning to be fired.
Of course, semicolon could be omitted, but the fact that the macro has braces is sort of an implementation detail.

Greg Parker has pointed out it’s possible to rewrite the macros using the “do { … } while (0)”,
but that is a lot of macros to change, and the resulting code would be arguably less readable.

That suggestion is the (or at least an) idiomatic way to write a "statement-like" macro that expects a semicolon. The approach taken by the above macros is often considered to be a macro antipattern (try putting it in an unbraced if and adding an else clause, for example).
On Dec 5, 2018, at 5:10 AM, Aaron Ballman <[hidden email]> wrote:

On Wed, Dec 5, 2018 at 1:40 AM Roman Lebedev <[hidden email]> wrote:

It is a problem in practice for us since we have projects which enable all available warnings and also enable “-Werror”.
Hm, they have asked for -Weverything and they got it. Seems to be
working as intended.
When in previous discussions elsewhere i have talked about
-Weverything, i was always
been told that it isn't supposed to be used like that (admittedly, i
*do* use it like that :)).

Could you explain how is this different from any other warning that is
considered pointless by the project?
Why not simply disable it?

You are right, it could be disabled. It’s just for this warning the noise vs. usefulness ratio seemed pretty low.


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.


Or rather when the *previous* semicolon is coming from a macro.
I don't like this. clang is already wa-a-ay too forgiving about
macros, it almost ignores almost every warning in macros.
It was an *intentional* decision to still warn on useless ';' after
non-empty macros.

I think I'm confused now. This is a test case:

NULLMACRO(v7); // OK. This could be either GOODMACRO() or
BETTERMACRO() situation, so we can't know we can drop it.

So empty macro expansions should be ignored as I expected... so what
is being diagnosed? George, can you provide a more clear example that
demonstrates the issue?

To be clear, I do *not* think this is a situation we should spend
effort supporting (assuming "all available warnings" really means
-Weverything and not something else):

It is a problem in practice for us since we have projects which enable all available warnings and also enable “-Werror”.

However, if we are diagnosing *empty* macro expansions followed by a
semi-colon, I think that would be unacceptably chatty and is worth
fixing on its own merits.

~Aaron


~Aaron

Regards,
George
Roman.

On Nov 20, 2018, at 10:59 AM, Roman Lebedev via cfe-commits <[hidden email]> 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
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev

_______________________________________________
cfe-commits mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

_______________________________________________
cfe-commits mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: r347339 - [clang][Parse] Diagnose useless null statements / empty init-statements

Oleg Smolsky via cfe-dev
We have about 100k unique occurrences of this warning across a slice of our software stack. It's simply not feasible to us to adopt it, so we would prefer for it to be reverted to avoid downstream divergence in Apple's clang. Granted, these occur in projects that use -Weverything but those projects are typically understanding of warnings that they can turn off when they see that they provide useful value in terms of warnings about unintended behavior that's sometimes caught. This warning doesn't seem to catch unintended bugs and it would be a heroic battle to adopt it for us in a way where projects are somewhat satisfied. It seems to be a better fit for a clang-tidy check that could suggest automatic fixits and that the users could run on their codebase to "modernize" if they want to.

Cheers,
Alex

On Wed, 5 Dec 2018 at 10:43, Richard Smith via cfe-dev <[hidden email]> wrote:
On Wed, 5 Dec 2018, 10:01 George Karpenkov via cfe-commits <[hidden email] wrote:
These are the cases I get:

#define unexpected(a) { kprintf("unexpected %s:%d\n", __FILE__, __LINE__); a; }

and

#if 0
#define DEBG(fmt, args...)      { IOLog(fmt, ## args); kprintf(fmt, ## args); }
#else
#define DEBG(fmt, args...)      {}
#endif

Now calls

`DEBG(‘x’);` and `unexpected(‘msg’);`

cause the warning to be fired.
Of course, semicolon could be omitted, but the fact that the macro has braces is sort of an implementation detail.

Greg Parker has pointed out it’s possible to rewrite the macros using the “do { … } while (0)”,
but that is a lot of macros to change, and the resulting code would be arguably less readable.

That suggestion is the (or at least an) idiomatic way to write a "statement-like" macro that expects a semicolon. The approach taken by the above macros is often considered to be a macro antipattern (try putting it in an unbraced if and adding an else clause, for example).
On Dec 5, 2018, at 5:10 AM, Aaron Ballman <[hidden email]> wrote:

On Wed, Dec 5, 2018 at 1:40 AM Roman Lebedev <[hidden email]> wrote:

It is a problem in practice for us since we have projects which enable all available warnings and also enable “-Werror”.
Hm, they have asked for -Weverything and they got it. Seems to be
working as intended.
When in previous discussions elsewhere i have talked about
-Weverything, i was always
been told that it isn't supposed to be used like that (admittedly, i
*do* use it like that :)).

Could you explain how is this different from any other warning that is
considered pointless by the project?
Why not simply disable it?

You are right, it could be disabled. It’s just for this warning the noise vs. usefulness ratio seemed pretty low.


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.


Or rather when the *previous* semicolon is coming from a macro.
I don't like this. clang is already wa-a-ay too forgiving about
macros, it almost ignores almost every warning in macros.
It was an *intentional* decision to still warn on useless ';' after
non-empty macros.

I think I'm confused now. This is a test case:

NULLMACRO(v7); // OK. This could be either GOODMACRO() or
BETTERMACRO() situation, so we can't know we can drop it.

So empty macro expansions should be ignored as I expected... so what
is being diagnosed? George, can you provide a more clear example that
demonstrates the issue?

To be clear, I do *not* think this is a situation we should spend
effort supporting (assuming "all available warnings" really means
-Weverything and not something else):

It is a problem in practice for us since we have projects which enable all available warnings and also enable “-Werror”.

However, if we are diagnosing *empty* macro expansions followed by a
semi-colon, I think that would be unacceptably chatty and is worth
fixing on its own merits.

~Aaron


~Aaron

Regards,
George
Roman.

On Nov 20, 2018, at 10:59 AM, Roman Lebedev via cfe-commits <[hidden email]> 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
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev

_______________________________________________
cfe-commits mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

_______________________________________________
cfe-commits mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev

_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: r347339 - [clang][Parse] Diagnose useless null statements / empty init-statements

Oleg Smolsky via cfe-dev
On Wed, Dec 5, 2018 at 2:14 PM Alex L via cfe-commits
<[hidden email]> wrote:
>
> We have about 100k unique occurrences of this warning across a slice of our software stack. It's simply not feasible to us to adopt it, so we would prefer for it to be reverted to avoid downstream divergence in Apple's clang. Granted, these occur in projects that use -Weverything but those projects are typically understanding of warnings that they can turn off when they see that they provide useful value in terms of warnings about unintended behavior that's sometimes caught. This warning doesn't seem to catch unintended bugs and it would be a heroic battle to adopt it for us in a way where projects are somewhat satisfied. It seems to be a better fit for a clang-tidy check that could suggest automatic fixits and that the users could run on their codebase to "modernize" if they want to.

I'm not certain I agree that this does not catch unintended bugs.
After all, it started a discussion that pointed out design issues with
two different macros which were provided as examples of
false-positives. I think this diagnostic points out bad code smells
and is no more stylistic than some of the other diagnostics in
-Weverything, like not having a previous prototype before defining a
function. I am not sold on the idea that this diagnostic should be
moved into clang-tidy simply because it's showing up in a lot of
-Weverything builds.

~Aaron

>
> Cheers,
> Alex
>
> On Wed, 5 Dec 2018 at 10:43, Richard Smith via cfe-dev <[hidden email]> wrote:
>>
>> On Wed, 5 Dec 2018, 10:01 George Karpenkov via cfe-commits <[hidden email] wrote:
>>>
>>> These are the cases I get:
>>>
>>> #define unexpected(a) { kprintf("unexpected %s:%d\n", __FILE__, __LINE__); a; }
>>>
>>> and
>>>
>>> #if 0
>>> #define DEBG(fmt, args...)      { IOLog(fmt, ## args); kprintf(fmt, ## args); }
>>> #else
>>> #define DEBG(fmt, args...)      {}
>>> #endif
>>>
>>> Now calls
>>>
>>> `DEBG(‘x’);` and `unexpected(‘msg’);`
>>>
>>> cause the warning to be fired.
>>> Of course, semicolon could be omitted, but the fact that the macro has braces is sort of an implementation detail.
>>>
>>> Greg Parker has pointed out it’s possible to rewrite the macros using the “do { … } while (0)”,
>>> but that is a lot of macros to change, and the resulting code would be arguably less readable.
>>
>>
>> That suggestion is the (or at least an) idiomatic way to write a "statement-like" macro that expects a semicolon. The approach taken by the above macros is often considered to be a macro antipattern (try putting it in an unbraced if and adding an else clause, for example).
>>>
>>> On Dec 5, 2018, at 5:10 AM, Aaron Ballman <[hidden email]> wrote:
>>>
>>> On Wed, Dec 5, 2018 at 1:40 AM Roman Lebedev <[hidden email]> wrote:
>>>
>>>
>>> It is a problem in practice for us since we have projects which enable all available warnings and also enable “-Werror”.
>>>
>>> Hm, they have asked for -Weverything and they got it. Seems to be
>>> working as intended.
>>> When in previous discussions elsewhere i have talked about
>>> -Weverything, i was always
>>> been told that it isn't supposed to be used like that (admittedly, i
>>> *do* use it like that :)).
>>>
>>> Could you explain how is this different from any other warning that is
>>> considered pointless by the project?
>>> Why not simply disable it?
>>>
>>>
>>> You are right, it could be disabled. It’s just for this warning the noise vs. usefulness ratio seemed pretty low.
>>>
>>>
>>> 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.
>>>
>>>
>>> Or rather when the *previous* semicolon is coming from a macro.
>>>
>>> I don't like this. clang is already wa-a-ay too forgiving about
>>> macros, it almost ignores almost every warning in macros.
>>> It was an *intentional* decision to still warn on useless ';' after
>>> non-empty macros.
>>>
>>>
>>> I think I'm confused now. This is a test case:
>>>
>>> NULLMACRO(v7); // OK. This could be either GOODMACRO() or
>>> BETTERMACRO() situation, so we can't know we can drop it.
>>>
>>> So empty macro expansions should be ignored as I expected... so what
>>> is being diagnosed? George, can you provide a more clear example that
>>> demonstrates the issue?
>>>
>>> To be clear, I do *not* think this is a situation we should spend
>>> effort supporting (assuming "all available warnings" really means
>>> -Weverything and not something else):
>>>
>>> It is a problem in practice for us since we have projects which enable all available warnings and also enable “-Werror”.
>>>
>>>
>>> However, if we are diagnosing *empty* macro expansions followed by a
>>> semi-colon, I think that would be unacceptably chatty and is worth
>>> fixing on its own merits.
>>>
>>> ~Aaron
>>>
>>>
>>> ~Aaron
>>>
>>> Regards,
>>> George
>>>
>>> Roman.
>>>
>>> On Nov 20, 2018, at 10:59 AM, Roman Lebedev via cfe-commits <[hidden email]> 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
>>> [hidden email]
>>> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
>>>
>>> _______________________________________________
>>> cfe-dev mailing list
>>> [hidden email]
>>> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
>>>
>>>
>>> _______________________________________________
>>> cfe-commits mailing list
>>> [hidden email]
>>> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
>>>
>>>
>>> _______________________________________________
>>> cfe-commits mailing list
>>> [hidden email]
>>> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
>>
>> _______________________________________________
>> cfe-dev mailing list
>> [hidden email]
>> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
>
> _______________________________________________
> cfe-commits mailing list
> [hidden email]
> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: r347339 - [clang][Parse] Diagnose useless null statements / empty init-statements

Oleg Smolsky via cfe-dev
Nobody should be using "-Weverything" in their default build flags! It should only be used as a way to find the names of interesting warning flags. I'd note that "-Weverything" even has warnings that _conflict_ with each-other...

It's unworkable for clang to have a policy which prevents the addition of new warning options just because someone enables -Weverything... If people feel like that is what's happening now, we need to fix that. If that's what it comes to, even deleting the -Weverything flag would be better.

The question I'd ask for new warnings is a different one: is this warning something which people could feasibly decide to opt into, with -Werror, for their entire project? This does appear to meet that criteria -- the cases where it warns do appear to be indicating potential issues, and the code fix which silences the warning will improve the code, not make it worse.

On Wed, Dec 5, 2018 at 2:51 PM Aaron Ballman via cfe-commits <[hidden email]> wrote:
On Wed, Dec 5, 2018 at 2:14 PM Alex L via cfe-commits
<[hidden email]> wrote:
>
> We have about 100k unique occurrences of this warning across a slice of our software stack. It's simply not feasible to us to adopt it, so we would prefer for it to be reverted to avoid downstream divergence in Apple's clang. Granted, these occur in projects that use -Weverything but those projects are typically understanding of warnings that they can turn off when they see that they provide useful value in terms of warnings about unintended behavior that's sometimes caught. This warning doesn't seem to catch unintended bugs and it would be a heroic battle to adopt it for us in a way where projects are somewhat satisfied. It seems to be a better fit for a clang-tidy check that could suggest automatic fixits and that the users could run on their codebase to "modernize" if they want to.

I'm not certain I agree that this does not catch unintended bugs.
After all, it started a discussion that pointed out design issues with
two different macros which were provided as examples of
false-positives. I think this diagnostic points out bad code smells
and is no more stylistic than some of the other diagnostics in
-Weverything, like not having a previous prototype before defining a
function. I am not sold on the idea that this diagnostic should be
moved into clang-tidy simply because it's showing up in a lot of
-Weverything builds.

~Aaron

>
> Cheers,
> Alex
>
> On Wed, 5 Dec 2018 at 10:43, Richard Smith via cfe-dev <[hidden email]> wrote:
>>
>> On Wed, 5 Dec 2018, 10:01 George Karpenkov via cfe-commits <[hidden email] wrote:
>>>
>>> These are the cases I get:
>>>
>>> #define unexpected(a) { kprintf("unexpected %s:%d\n", __FILE__, __LINE__); a; }
>>>
>>> and
>>>
>>> #if 0
>>> #define DEBG(fmt, args...)      { IOLog(fmt, ## args); kprintf(fmt, ## args); }
>>> #else
>>> #define DEBG(fmt, args...)      {}
>>> #endif
>>>
>>> Now calls
>>>
>>> `DEBG(‘x’);` and `unexpected(‘msg’);`
>>>
>>> cause the warning to be fired.
>>> Of course, semicolon could be omitted, but the fact that the macro has braces is sort of an implementation detail.
>>>
>>> Greg Parker has pointed out it’s possible to rewrite the macros using the “do { … } while (0)”,
>>> but that is a lot of macros to change, and the resulting code would be arguably less readable.
>>
>>
>> That suggestion is the (or at least an) idiomatic way to write a "statement-like" macro that expects a semicolon. The approach taken by the above macros is often considered to be a macro antipattern (try putting it in an unbraced if and adding an else clause, for example).
>>>
>>> On Dec 5, 2018, at 5:10 AM, Aaron Ballman <[hidden email]> wrote:
>>>
>>> On Wed, Dec 5, 2018 at 1:40 AM Roman Lebedev <[hidden email]> wrote:
>>>
>>>
>>> It is a problem in practice for us since we have projects which enable all available warnings and also enable “-Werror”.
>>>
>>> Hm, they have asked for -Weverything and they got it. Seems to be
>>> working as intended.
>>> When in previous discussions elsewhere i have talked about
>>> -Weverything, i was always
>>> been told that it isn't supposed to be used like that (admittedly, i
>>> *do* use it like that :)).
>>>
>>> Could you explain how is this different from any other warning that is
>>> considered pointless by the project?
>>> Why not simply disable it?
>>>
>>>
>>> You are right, it could be disabled. It’s just for this warning the noise vs. usefulness ratio seemed pretty low.
>>>
>>>
>>> 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.
>>>
>>>
>>> Or rather when the *previous* semicolon is coming from a macro.
>>>
>>> I don't like this. clang is already wa-a-ay too forgiving about
>>> macros, it almost ignores almost every warning in macros.
>>> It was an *intentional* decision to still warn on useless ';' after
>>> non-empty macros.
>>>
>>>
>>> I think I'm confused now. This is a test case:
>>>
>>> NULLMACRO(v7); // OK. This could be either GOODMACRO() or
>>> BETTERMACRO() situation, so we can't know we can drop it.
>>>
>>> So empty macro expansions should be ignored as I expected... so what
>>> is being diagnosed? George, can you provide a more clear example that
>>> demonstrates the issue?
>>>
>>> To be clear, I do *not* think this is a situation we should spend
>>> effort supporting (assuming "all available warnings" really means
>>> -Weverything and not something else):
>>>
>>> It is a problem in practice for us since we have projects which enable all available warnings and also enable “-Werror”.
>>>
>>>
>>> However, if we are diagnosing *empty* macro expansions followed by a
>>> semi-colon, I think that would be unacceptably chatty and is worth
>>> fixing on its own merits.
>>>
>>> ~Aaron
>>>
>>>
>>> ~Aaron
>>>
>>> Regards,
>>> George
>>>
>>> Roman.
>>>
>>> On Nov 20, 2018, at 10:59 AM, Roman Lebedev via cfe-commits <[hidden email]> 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
>>> [hidden email]
>>> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
>>>
>>> _______________________________________________
>>> cfe-dev mailing list
>>> [hidden email]
>>> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
>>>
>>>
>>> _______________________________________________
>>> cfe-commits mailing list
>>> [hidden email]
>>> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
>>>
>>>
>>> _______________________________________________
>>> cfe-commits mailing list
>>> [hidden email]
>>> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
>>
>> _______________________________________________
>> cfe-dev mailing list
>> [hidden email]
>> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
>
> _______________________________________________
> cfe-commits mailing list
> [hidden email]
> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
_______________________________________________
cfe-commits mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: r347339 - [clang][Parse] Diagnose useless null statements / empty init-statements

Oleg Smolsky via cfe-dev
In reply to this post by Oleg Smolsky via cfe-dev
On Wed, 5 Dec 2018 at 11:14, Alex L via cfe-commits <[hidden email]> wrote:
We have about 100k unique occurrences of this warning across a slice of our software stack. It's simply not feasible to us to adopt it, so we would prefer for it to be reverted to avoid downstream divergence in Apple's clang. Granted, these occur in projects that use -Weverything but those projects are typically understanding of warnings that they can turn off when they see that they provide useful value in terms of warnings about unintended behavior that's sometimes caught. This warning doesn't seem to catch unintended bugs and it would be a heroic battle to adopt it for us in a way where projects are somewhat satisfied. It seems to be a better fit for a clang-tidy check that could suggest automatic fixits and that the users could run on their codebase to "modernize" if they want to.

Please refer to the review thread where -Weverything was added:


Users using -Weverything have always been expected to turn off warnings that they find not useful for their use cases (or to not use -Weverything if they cannot tolerate new warnings) -- in fact, the very model for -Weverything is that you see new warnings and turn off the ones you don't like. That's the model that users explicitly opt into when they ask for -Weveryhing. It would be harmful to allow only lowest-common-denominator warnings in order to avoid breaking people who incautiously opted into -Weverything -Werror, so the fact that a disabled-by-default warning is problematic for such users is not a compelling reason to remove the warning.

If the warning itself does not objectively have merit, then we can remove it, but the fact that it breaks -Weverything -Werror builds shouldn't be a consideration.

Cheers,
Alex

On Wed, 5 Dec 2018 at 10:43, Richard Smith via cfe-dev <[hidden email]> wrote:
On Wed, 5 Dec 2018, 10:01 George Karpenkov via cfe-commits <[hidden email] wrote:
These are the cases I get:

#define unexpected(a) { kprintf("unexpected %s:%d\n", __FILE__, __LINE__); a; }

and

#if 0
#define DEBG(fmt, args...)      { IOLog(fmt, ## args); kprintf(fmt, ## args); }
#else
#define DEBG(fmt, args...)      {}
#endif

Now calls

`DEBG(‘x’);` and `unexpected(‘msg’);`

cause the warning to be fired.
Of course, semicolon could be omitted, but the fact that the macro has braces is sort of an implementation detail.

Greg Parker has pointed out it’s possible to rewrite the macros using the “do { … } while (0)”,
but that is a lot of macros to change, and the resulting code would be arguably less readable.

That suggestion is the (or at least an) idiomatic way to write a "statement-like" macro that expects a semicolon. The approach taken by the above macros is often considered to be a macro antipattern (try putting it in an unbraced if and adding an else clause, for example).
On Dec 5, 2018, at 5:10 AM, Aaron Ballman <[hidden email]> wrote:

On Wed, Dec 5, 2018 at 1:40 AM Roman Lebedev <[hidden email]> wrote:

It is a problem in practice for us since we have projects which enable all available warnings and also enable “-Werror”.
Hm, they have asked for -Weverything and they got it. Seems to be
working as intended.
When in previous discussions elsewhere i have talked about
-Weverything, i was always
been told that it isn't supposed to be used like that (admittedly, i
*do* use it like that :)).

Could you explain how is this different from any other warning that is
considered pointless by the project?
Why not simply disable it?

You are right, it could be disabled. It’s just for this warning the noise vs. usefulness ratio seemed pretty low.


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.


Or rather when the *previous* semicolon is coming from a macro.
I don't like this. clang is already wa-a-ay too forgiving about
macros, it almost ignores almost every warning in macros.
It was an *intentional* decision to still warn on useless ';' after
non-empty macros.

I think I'm confused now. This is a test case:

NULLMACRO(v7); // OK. This could be either GOODMACRO() or
BETTERMACRO() situation, so we can't know we can drop it.

So empty macro expansions should be ignored as I expected... so what
is being diagnosed? George, can you provide a more clear example that
demonstrates the issue?

To be clear, I do *not* think this is a situation we should spend
effort supporting (assuming "all available warnings" really means
-Weverything and not something else):

It is a problem in practice for us since we have projects which enable all available warnings and also enable “-Werror”.

However, if we are diagnosing *empty* macro expansions followed by a
semi-colon, I think that would be unacceptably chatty and is worth
fixing on its own merits.

~Aaron


~Aaron

Regards,
George
Roman.

On Nov 20, 2018, at 10:59 AM, Roman Lebedev via cfe-commits <[hidden email]> 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
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev

_______________________________________________
cfe-commits mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

_______________________________________________
cfe-commits mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
_______________________________________________
cfe-commits mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev