[cfe-dev] r347339 - [clang][Parse] Diagnose useless null statements / empty init-statements

Richard Smith via cfe-dev cfe-dev at lists.llvm.org
Thu Dec 6 13:10:03 PST 2018


On Wed, 5 Dec 2018 at 11:14, Alex L via cfe-commits <
cfe-commits at lists.llvm.org> 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:


http://lists.llvm.org/pipermail/cfe-commits/Week-of-Mon-20110815/045292.html

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 <
> cfe-dev at lists.llvm.org> wrote:
>
>> On Wed, 5 Dec 2018, 10:01 George Karpenkov via cfe-commits <
>> cfe-commits at lists.llvm.org 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 <aaron at aaronballman.com>
>>> wrote:
>>>
>>> On Wed, Dec 5, 2018 at 1:40 AM Roman Lebedev <lebedev.ri at gmail.com>
>>> 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 <
>>> cfe-commits at lists.llvm.org> wrote:
>>>
>>> Author: lebedevri
>>> Date: Tue Nov 20 10:59:05 2018
>>> New Revision: 347339
>>>
>>> URL: http://llvm.org/viewvc/llvm-project?rev=347339&view=rev
>>> Log:
>>> [clang][Parse] Diagnose useless null statements / empty init-statements
>>>
>>> Summary:
>>> clang has `-Wextra-semi` (D43162), which is not dictated by the
>>> currently selected standard.
>>> While that is great, there is at least one more source of need-less
>>> semis - 'null statements'.
>>> Sometimes, they are needed:
>>> ```
>>> for(int x = 0; continueToDoWork(x); x++)
>>> ; // Ugly code, but the semi is needed here.
>>> ```
>>>
>>> But sometimes they are just there for no reason:
>>> ```
>>> switch(X) {
>>> case 0:
>>> return -2345;
>>> case 5:
>>> return 0;
>>> default:
>>> return 42;
>>> }; // <- oops
>>>
>>> ;;;;;;;;;;; <- OOOOPS, still not diagnosed. Clearly this is junk.
>>> ```
>>>
>>> Additionally:
>>> ```
>>> if(; // <- empty init-statement
>>>  true)
>>> ;
>>>
>>> switch (; // empty init-statement
>>>       x) {
>>> ...
>>> }
>>>
>>> for (; // <- empty init-statement
>>>    int y : S())
>>> ;
>>> }
>>>
>>> As usual, things may or may not go sideways in the presence of macros.
>>> While evaluating this diag on my codebase of interest, it was
>>> unsurprisingly
>>> discovered that Google Test macros are *very* prone to this.
>>> And it seems many issues are deep within the GTest itself, not
>>> in the snippets passed from the codebase that uses GTest.
>>>
>>> So after some thought, i decided not do issue a diagnostic if the semi
>>> is within *any* macro, be it either from the normal header, or system
>>> header.
>>>
>>> Fixes [[ https://bugs.llvm.org/show_bug.cgi?id=39111 | PR39111 ]]
>>>
>>> Reviewers: rsmith, aaron.ballman, efriedma
>>>
>>> Reviewed By: aaron.ballman
>>>
>>> Subscribers: cfe-commits
>>>
>>> Differential Revision: https://reviews.llvm.org/D52695
>>>
>>> Added:
>>>
>>>   cfe/trunk/test/Parser/extra-semi-resulting-in-nullstmt-in-init-statement.cpp
>>>   cfe/trunk/test/Parser/extra-semi-resulting-in-nullstmt.cpp
>>> Modified:
>>>   cfe/trunk/docs/ReleaseNotes.rst
>>>   cfe/trunk/include/clang/Basic/DiagnosticGroups.td
>>>   cfe/trunk/include/clang/Basic/DiagnosticParseKinds.td
>>>   cfe/trunk/include/clang/Parse/Parser.h
>>>   cfe/trunk/lib/Parse/ParseExprCXX.cpp
>>>   cfe/trunk/lib/Parse/ParseStmt.cpp
>>>
>>> Modified: cfe/trunk/docs/ReleaseNotes.rst
>>> URL:
>>> http://llvm.org/viewvc/llvm-project/cfe/trunk/docs/ReleaseNotes.rst?rev=347339&r1=347338&r2=347339&view=diff
>>>
>>> ==============================================================================
>>> --- cfe/trunk/docs/ReleaseNotes.rst (original)
>>> +++ cfe/trunk/docs/ReleaseNotes.rst Tue Nov 20 10:59:05 2018
>>> @@ -55,6 +55,63 @@ Major New Features
>>> Improvements to Clang's diagnostics
>>> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>>>
>>> +- ``-Wextra-semi-stmt`` is a new diagnostic that diagnoses extra
>>> semicolons,
>>> +  much like ``-Wextra-semi``. This new diagnostic diagnoses all
>>> *unnecessary*
>>> +  null statements (expression statements without an expression),
>>> unless: the
>>> +  semicolon directly follows a macro that was expanded to nothing or if
>>> the
>>> +  semicolon is within the macro itself. This applies to macros defined
>>> in system
>>> +  headers as well as user-defined macros.
>>> +
>>> +  .. code-block:: c++
>>> +
>>> +      #define MACRO(x) int x;
>>> +      #define NULLMACRO(varname)
>>> +
>>> +      void test() {
>>> +        ; // <- warning: ';' with no preceding expression is a null
>>> statement
>>> +
>>> +        while (true)
>>> +          ; // OK, it is needed.
>>> +
>>> +        switch (my_enum) {
>>> +        case E1:
>>> +          // stuff
>>> +          break;
>>> +        case E2:
>>> +          ; // OK, it is needed.
>>> +        }
>>> +
>>> +        MACRO(v0;) // Extra semicolon, but within macro, so ignored.
>>> +
>>> +        MACRO(v1); // <- warning: ';' with no preceding expression is a
>>> null statement
>>> +
>>> +        NULLMACRO(v2); // ignored, NULLMACRO expanded to nothing.
>>> +      }
>>> +
>>> +- ``-Wempty-init-stmt`` is a new diagnostic that diagnoses empty
>>> init-statements
>>> +  of ``if``, ``switch``, ``range-based for``, unless: the semicolon
>>> directly
>>> +  follows a macro that was expanded to nothing or if the semicolon is
>>> within the
>>> +  macro itself (both macros from system headers, and normal macros).
>>> This
>>> +  diagnostic is in the ``-Wextra-semi-stmt`` group and is enabled in
>>> +  ``-Wextra``.
>>> +
>>> +  .. code-block:: c++
>>> +
>>> +      void test() {
>>> +        if(; // <- warning: init-statement of 'if' is a null statement
>>> +           true)
>>> +          ;
>>> +
>>> +        switch (; // <- warning: init-statement of 'switch' is a null
>>> statement
>>> +                x) {
>>> +          ...
>>> +        }
>>> +
>>> +        for (; // <- warning: init-statement of 'range-based for' is a
>>> null statement
>>> +             int y : S())
>>> +          ;
>>> +      }
>>> +
>>>
>>> Non-comprehensive list of changes in this release
>>> -------------------------------------------------
>>>
>>> Modified: cfe/trunk/include/clang/Basic/DiagnosticGroups.td
>>> URL:
>>> http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Basic/DiagnosticGroups.td?rev=347339&r1=347338&r2=347339&view=diff
>>>
>>> ==============================================================================
>>> --- cfe/trunk/include/clang/Basic/DiagnosticGroups.td (original)
>>> +++ cfe/trunk/include/clang/Basic/DiagnosticGroups.td Tue Nov 20
>>> 10:59:05 2018
>>> @@ -162,6 +162,8 @@ def GNUEmptyStruct : DiagGroup<"gnu-empt
>>> def ExtraTokens : DiagGroup<"extra-tokens">;
>>> def CXX98CompatExtraSemi : DiagGroup<"c++98-compat-extra-semi">;
>>> def CXX11ExtraSemi : DiagGroup<"c++11-extra-semi">;
>>> +def EmptyInitStatement : DiagGroup<"empty-init-stmt">;
>>> +def ExtraSemiStmt : DiagGroup<"extra-semi-stmt", [EmptyInitStatement]>;
>>> def ExtraSemi : DiagGroup<"extra-semi", [CXX98CompatExtraSemi,
>>>                                         CXX11ExtraSemi]>;
>>>
>>> @@ -768,7 +770,8 @@ def Extra : DiagGroup<"extra", [
>>>    MissingMethodReturnType,
>>>    SignCompare,
>>>    UnusedParameter,
>>> -    NullPointerArithmetic
>>> +    NullPointerArithmetic,
>>> +    EmptyInitStatement
>>>  ]>;
>>>
>>> def Most : DiagGroup<"most", [
>>>
>>> Modified: cfe/trunk/include/clang/Basic/DiagnosticParseKinds.td
>>> URL:
>>> http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Basic/DiagnosticParseKinds.td?rev=347339&r1=347338&r2=347339&view=diff
>>>
>>> ==============================================================================
>>> --- cfe/trunk/include/clang/Basic/DiagnosticParseKinds.td (original)
>>> +++ cfe/trunk/include/clang/Basic/DiagnosticParseKinds.td Tue Nov 20
>>> 10:59:05 2018
>>> @@ -53,6 +53,10 @@ def ext_extra_semi_cxx11 : Extension<
>>> def warn_extra_semi_after_mem_fn_def : Warning<
>>>  "extra ';' after member function definition">,
>>>  InGroup<ExtraSemi>, DefaultIgnore;
>>> +def warn_null_statement : Warning<
>>> +  "empty expression statement has no effect; "
>>> +  "remove unnecessary ';' to silence this warning">,
>>> +  InGroup<ExtraSemiStmt>, DefaultIgnore;
>>>
>>> def ext_thread_before : Extension<"'__thread' before '%0'">;
>>> def ext_keyword_as_ident : ExtWarn<
>>> @@ -554,6 +558,9 @@ def ext_for_range_init_stmt : ExtWarn<
>>> def warn_cxx17_compat_for_range_init_stmt : Warning<
>>>  "range-based for loop initialization statements are incompatible with "
>>>  "C++ standards before C++2a">, DefaultIgnore, InGroup<CXXPre2aCompat>;
>>> +def warn_empty_init_statement : Warning<
>>> +  "empty initialization statement of '%select{if|switch|range-based
>>> for}0' "
>>> +  "has no effect">, InGroup<EmptyInitStatement>, DefaultIgnore;
>>>
>>> // C++ derived classes
>>> def err_dup_virtual : Error<"duplicate 'virtual' in base specifier">;
>>>
>>> Modified: cfe/trunk/include/clang/Parse/Parser.h
>>> URL:
>>> http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Parse/Parser.h?rev=347339&r1=347338&r2=347339&view=diff
>>>
>>> ==============================================================================
>>> --- cfe/trunk/include/clang/Parse/Parser.h (original)
>>> +++ cfe/trunk/include/clang/Parse/Parser.h Tue Nov 20 10:59:05 2018
>>> @@ -1888,6 +1888,7 @@ private:
>>>  StmtResult ParseCompoundStatement(bool isStmtExpr,
>>>                                    unsigned ScopeFlags);
>>>  void ParseCompoundStatementLeadingPragmas();
>>> +  bool ConsumeNullStmt(StmtVector &Stmts);
>>>  StmtResult ParseCompoundStatementBody(bool isStmtExpr = false);
>>>  bool ParseParenExprOrCondition(StmtResult *InitStmt,
>>>                                 Sema::ConditionResult &CondResult,
>>>
>>> Modified: cfe/trunk/lib/Parse/ParseExprCXX.cpp
>>> URL:
>>> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Parse/ParseExprCXX.cpp?rev=347339&r1=347338&r2=347339&view=diff
>>>
>>> ==============================================================================
>>> --- cfe/trunk/lib/Parse/ParseExprCXX.cpp (original)
>>> +++ cfe/trunk/lib/Parse/ParseExprCXX.cpp Tue Nov 20 10:59:05 2018
>>> @@ -1773,7 +1773,13 @@ Sema::ConditionResult Parser::ParseCXXCo
>>>    //   if (; true);
>>>    if (InitStmt && Tok.is(tok::semi)) {
>>>      WarnOnInit();
>>> -      SourceLocation SemiLoc = ConsumeToken();
>>> +      SourceLocation SemiLoc = Tok.getLocation();
>>> +      if (!Tok.hasLeadingEmptyMacro() && !SemiLoc.isMacroID()) {
>>> +        Diag(SemiLoc, diag::warn_empty_init_statement)
>>> +            << (CK == Sema::ConditionKind::Switch)
>>> +            << FixItHint::CreateRemoval(SemiLoc);
>>> +      }
>>> +      ConsumeToken();
>>>      *InitStmt = Actions.ActOnNullStmt(SemiLoc);
>>>      return ParseCXXCondition(nullptr, Loc, CK);
>>>    }
>>>
>>> Modified: cfe/trunk/lib/Parse/ParseStmt.cpp
>>> URL:
>>> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Parse/ParseStmt.cpp?rev=347339&r1=347338&r2=347339&view=diff
>>>
>>> ==============================================================================
>>> --- cfe/trunk/lib/Parse/ParseStmt.cpp (original)
>>> +++ cfe/trunk/lib/Parse/ParseStmt.cpp Tue Nov 20 10:59:05 2018
>>> @@ -930,6 +930,34 @@ void Parser::ParseCompoundStatementLeadi
>>>
>>> }
>>>
>>> +/// Consume any extra semi-colons resulting in null statements,
>>> +/// returning true if any tok::semi were consumed.
>>> +bool Parser::ConsumeNullStmt(StmtVector &Stmts) {
>>> +  if (!Tok.is(tok::semi))
>>> +    return false;
>>> +
>>> +  SourceLocation StartLoc = Tok.getLocation();
>>> +  SourceLocation EndLoc;
>>> +
>>> +  while (Tok.is(tok::semi) && !Tok.hasLeadingEmptyMacro() &&
>>> +         Tok.getLocation().isValid() && !Tok.getLocation().isMacroID())
>>> {
>>> +    EndLoc = Tok.getLocation();
>>> +
>>> +    // Don't just ConsumeToken() this tok::semi, do store it in AST.
>>> +    StmtResult R = ParseStatementOrDeclaration(Stmts, ACK_Any);
>>> +    if (R.isUsable())
>>> +      Stmts.push_back(R.get());
>>> +  }
>>> +
>>> +  // Did not consume any extra semi.
>>> +  if (EndLoc.isInvalid())
>>> +    return false;
>>> +
>>> +  Diag(StartLoc, diag::warn_null_statement)
>>> +      << FixItHint::CreateRemoval(SourceRange(StartLoc, EndLoc));
>>> +  return true;
>>> +}
>>> +
>>> /// ParseCompoundStatementBody - Parse a sequence of statements and
>>> invoke the
>>> /// ActOnCompoundStmt action.  This expects the '{' to be the current
>>> token, and
>>> /// consume the '}' at the end of the block.  It does not manipulate the
>>> scope
>>> @@ -992,6 +1020,9 @@ StmtResult Parser::ParseCompoundStatemen
>>>      continue;
>>>    }
>>>
>>> +    if (ConsumeNullStmt(Stmts))
>>> +      continue;
>>> +
>>>    StmtResult R;
>>>    if (Tok.isNot(tok::kw___extension__)) {
>>>      R = ParseStatementOrDeclaration(Stmts, ACK_Any);
>>> @@ -1588,10 +1619,15 @@ StmtResult Parser::ParseForStatement(Sou
>>>  ParsedAttributesWithRange attrs(AttrFactory);
>>>  MaybeParseCXX11Attributes(attrs);
>>>
>>> +  SourceLocation EmptyInitStmtSemiLoc;
>>> +
>>>  // Parse the first part of the for specifier.
>>>  if (Tok.is(tok::semi)) {  // for (;
>>>    ProhibitAttributes(attrs);
>>>    // no first part, eat the ';'.
>>> +    SourceLocation SemiLoc = Tok.getLocation();
>>> +    if (!Tok.hasLeadingEmptyMacro() && !SemiLoc.isMacroID())
>>> +      EmptyInitStmtSemiLoc = SemiLoc;
>>>    ConsumeToken();
>>>  } else if (getLangOpts().CPlusPlus && Tok.is(tok::identifier) &&
>>>             isForRangeIdentifier()) {
>>> @@ -1723,6 +1759,11 @@ StmtResult Parser::ParseForStatement(Sou
>>>                   : diag::ext_for_range_init_stmt)
>>>              << (FirstPart.get() ? FirstPart.get()->getSourceRange()
>>>                                  : SourceRange());
>>> +          if (EmptyInitStmtSemiLoc.isValid()) {
>>> +            Diag(EmptyInitStmtSemiLoc, diag::warn_empty_init_statement)
>>> +                << /*for-loop*/ 2
>>> +                << FixItHint::CreateRemoval(EmptyInitStmtSemiLoc);
>>> +          }
>>>        }
>>>      } else {
>>>        ExprResult SecondExpr = ParseExpression();
>>>
>>> Added:
>>> cfe/trunk/test/Parser/extra-semi-resulting-in-nullstmt-in-init-statement.cpp
>>> URL:
>>> http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Parser/extra-semi-resulting-in-nullstmt-in-init-statement.cpp?rev=347339&view=auto
>>>
>>> ==============================================================================
>>> ---
>>> cfe/trunk/test/Parser/extra-semi-resulting-in-nullstmt-in-init-statement.cpp
>>> (added)
>>> +++
>>> cfe/trunk/test/Parser/extra-semi-resulting-in-nullstmt-in-init-statement.cpp
>>> Tue Nov 20 10:59:05 2018
>>> @@ -0,0 +1,64 @@
>>> +// RUN: %clang_cc1 -fsyntax-only -Wextra -std=c++2a -verify %s
>>> +// RUN: %clang_cc1 -fsyntax-only -Wextra-semi-stmt -std=c++2a -verify %s
>>> +// RUN: %clang_cc1 -fsyntax-only -Wempty-init-stmt -std=c++2a -verify %s
>>> +// RUN: cp %s %t
>>> +// RUN: %clang_cc1 -x c++ -Wempty-init-stmt -std=c++2a -fixit %t
>>> +// RUN: %clang_cc1 -x c++ -Wempty-init-stmt -std=c++2a -Werror %t
>>> +
>>> +struct S {
>>> +  int *begin();
>>> +  int *end();
>>> +};
>>> +
>>> +void naive(int x) {
>>> +  if (; true) // expected-warning {{empty initialization statement of
>>> 'if' has no effect}}
>>> +    ;
>>> +
>>> +  switch (; x) { // expected-warning {{empty initialization statement
>>> of 'switch' has no effect}}
>>> +  }
>>> +
>>> +  for (; int y : S()) // expected-warning {{empty initialization
>>> statement of 'range-based for' has no effect}}
>>> +    ;
>>> +
>>> +  for (;;) // OK
>>> +    ;
>>> +}
>>> +
>>> +#define NULLMACRO
>>> +
>>> +void with_null_macro(int x) {
>>> +  if (NULLMACRO; true)
>>> +    ;
>>> +
>>> +  switch (NULLMACRO; x) {
>>> +  }
>>> +
>>> +  for (NULLMACRO; int y : S())
>>> +    ;
>>> +}
>>> +
>>> +#define SEMIMACRO ;
>>> +
>>> +void with_semi_macro(int x) {
>>> +  if (SEMIMACRO true)
>>> +    ;
>>> +
>>> +  switch (SEMIMACRO x) {
>>> +  }
>>> +
>>> +  for (SEMIMACRO int y : S())
>>> +    ;
>>> +}
>>> +
>>> +#define PASSTHROUGHMACRO(x) x
>>> +
>>> +void with_passthrough_macro(int x) {
>>> +  if (PASSTHROUGHMACRO(;) true)
>>> +    ;
>>> +
>>> +  switch (PASSTHROUGHMACRO(;) x) {
>>> +  }
>>> +
>>> +  for (PASSTHROUGHMACRO(;) int y : S())
>>> +    ;
>>> +}
>>>
>>> Added: cfe/trunk/test/Parser/extra-semi-resulting-in-nullstmt.cpp
>>> URL:
>>> http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Parser/extra-semi-resulting-in-nullstmt.cpp?rev=347339&view=auto
>>>
>>> ==============================================================================
>>> --- cfe/trunk/test/Parser/extra-semi-resulting-in-nullstmt.cpp (added)
>>> +++ cfe/trunk/test/Parser/extra-semi-resulting-in-nullstmt.cpp Tue Nov
>>> 20 10:59:05 2018
>>> @@ -0,0 +1,96 @@
>>> +// RUN: %clang_cc1 -fsyntax-only -Wextra-semi-stmt -verify %s
>>> +// RUN: cp %s %t
>>> +// RUN: %clang_cc1 -x c++ -Wextra-semi-stmt -fixit %t
>>> +// RUN: %clang_cc1 -x c++ -Wextra-semi-stmt -Werror %t
>>> +
>>> +#define GOODMACRO(varname) int varname
>>> +#define BETTERMACRO(varname) GOODMACRO(varname);
>>> +#define NULLMACRO(varname)
>>> +
>>> +enum MyEnum {
>>> +  E1,
>>> +  E2
>>> +};
>>> +
>>> +void test() {
>>> +  ; // expected-warning {{empty expression statement has no effect;
>>> remove unnecessary ';' to silence this warning}}
>>> +  ;
>>> +
>>> +  // This removal of extra semi also consumes all the comments.
>>> +  // clang-format: off
>>> +  ;;;
>>> +  // clang-format: on
>>> +
>>> +  // clang-format: off
>>> +  ;NULLMACRO(ZZ);
>>> +  // clang-format: on
>>> +
>>> +  {}; // expected-warning {{empty expression statement has no effect;
>>> remove unnecessary ';' to silence this warning}}
>>> +
>>> +  {
>>> +    ; // expected-warning {{empty expression statement has no effect;
>>> remove unnecessary ';' to silence this warning}}
>>> +  }
>>> +
>>> +  if (true) {
>>> +    ; // expected-warning {{empty expression statement has no effect;
>>> remove unnecessary ';' to silence this warning}}
>>> +  }
>>> +
>>> +  GOODMACRO(v0); // OK
>>> +
>>> +  GOODMACRO(v1;) // OK
>>> +
>>> +  BETTERMACRO(v2) // OK
>>> +
>>> +  BETTERMACRO(v3;) // Extra ';', but within macro expansion, so ignored.
>>> +
>>> +  BETTERMACRO(v4); // expected-warning {{empty expression statement has
>>> no effect; remove unnecessary ';' to silence this warning}}
>>> +
>>> +  BETTERMACRO(v5;); // expected-warning {{empty expression statement
>>> has no effect; remove unnecessary ';' to silence this warning}}
>>> +
>>> +  NULLMACRO(v6) // OK
>>> +
>>> +  NULLMACRO(v7); // OK. This could be either GOODMACRO() or
>>> BETTERMACRO() situation, so we can't know we can drop it.
>>> +
>>> +  if (true)
>>> +    ; // OK
>>> +
>>> +  while (true)
>>> +    ; // OK
>>> +
>>> +  do
>>> +    ; // OK
>>> +  while (true);
>>> +
>>> +  for (;;) // OK
>>> +    ;      // OK
>>> +
>>> +  MyEnum my_enum;
>>> +  switch (my_enum) {
>>> +  case E1:
>>> +    // stuff
>>> +    break;
>>> +  case E2:; // OK
>>> +  }
>>> +
>>> +  for (;;) {
>>> +    for (;;) {
>>> +      goto contin_outer;
>>> +    }
>>> +  contin_outer:; // OK
>>> +  }
>>> +}
>>> +
>>> +;
>>> +
>>> +namespace NS {};
>>> +
>>> +void foo(int x) {
>>> +  switch (x) {
>>> +  case 0:
>>> +    [[fallthrough]];
>>> +  case 1:
>>> +    return;
>>> +  }
>>> +}
>>> +
>>> +[[]];
>>>
>>>
>>> _______________________________________________
>>> cfe-commits mailing list
>>> cfe-commits at lists.llvm.org
>>> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
>>>
>>> _______________________________________________
>>> cfe-dev mailing list
>>> cfe-dev at lists.llvm.org
>>> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
>>>
>>>
>>> _______________________________________________
>>> cfe-commits mailing list
>>> cfe-commits at lists.llvm.org
>>> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
>>>
>>>
>>> _______________________________________________
>>> cfe-commits mailing list
>>> cfe-commits at lists.llvm.org
>>> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
>>>
>> _______________________________________________
>> cfe-dev mailing list
>> cfe-dev at lists.llvm.org
>> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
>>
> _______________________________________________
> cfe-commits mailing list
> cfe-commits at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20181206/93f921b5/attachment-0001.html>


More information about the cfe-dev mailing list