[cfe-commits] [PATCH] Properly check for unused comparisons in arguments to function-like macros

[Matt Beaumont-Gay]

Ping.

On Thu, Jan 3, 2013 at 1:28 PM, Matt Beaumont-Gay <matthewbg at google.com> wrote:
> I just realized this might seem a little weird, since we'll still give
> a -Wunused-value warning for an unused comparison in a macro body.
> But, my next patch is going to be "suppress all -Wunused-value
> warnings from expressions in macro bodies" -- these are the biggest
> source of false positives from -Wunused-value in Google's codebase.
>
> On Wed, Jan 2, 2013 at 5:10 PM, Matt Beaumont-Gay <matthewbg at google.com> wrote:
>> Hi chandlerc,
>>
>> Tweaking the implementation of -Wunused-value, I noticed a misbehavior of -Wunused-comparison: If the comparison is in a macro argument, we give up on emitting -Wunused-comparison, but fall through to emitting -Wunused-value. This patch fixes -Wunused-comparison to do the right thing. I also added a helper function 'isMacroBodyExpansion' to SourceManager (to go along with 'isMacroArgExpansion').
>>
>> http://llvm-reviews.chandlerc.com/D259
>>
>> Files:
>>   include/clang/Basic/SourceManager.h
>>   lib/Basic/SourceManager.cpp
>>   lib/Sema/SemaStmt.cpp
>>   test/Sema/unused-expr-system-header.c
>>
>> Index: include/clang/Basic/SourceManager.h
>> ===================================================================
>> --- include/clang/Basic/SourceManager.h
>> +++ include/clang/Basic/SourceManager.h
>> @@ -329,6 +329,11 @@
>>          SourceLocation::getFromRawEncoding(ExpansionLocEnd).isInvalid();
>>      }
>>
>> +    bool isMacroBodyExpansion() const {
>> +      return getExpansionLocStart().isValid() &&
>> +        SourceLocation::getFromRawEncoding(ExpansionLocEnd).isValid();
>> +    }
>> +
>>      bool isFunctionMacroExpansion() const {
>>        return getExpansionLocStart().isValid() &&
>>            getExpansionLocStart() != getExpansionLocEnd();
>> @@ -1126,6 +1131,11 @@
>>    /// expanded.
>>    bool isMacroArgExpansion(SourceLocation Loc) const;
>>
>> +  /// \brief Tests whether the given source location represents the expansion of
>> +  /// a macro body (as opposed to an expansion of the argument to a
>> +  /// function-like macro).
>> +  bool isMacroBodyExpansion(SourceLocation Loc) const;
>> +
>>    /// \brief Returns true if \p Loc is inside the [\p Start, +\p Length)
>>    /// chunk of the source location address space.
>>    ///
>> Index: lib/Basic/SourceManager.cpp
>> ===================================================================
>> --- lib/Basic/SourceManager.cpp
>> +++ lib/Basic/SourceManager.cpp
>> @@ -979,6 +979,15 @@
>>    return Expansion.isMacroArgExpansion();
>>  }
>>
>> +bool SourceManager::isMacroBodyExpansion(SourceLocation Loc) const {
>> +  if (!Loc.isMacroID()) return false;
>> +
>> +  FileID FID = getFileID(Loc);
>> +  const SrcMgr::SLocEntry *E = &getSLocEntry(FID);
>> +  const SrcMgr::ExpansionInfo &Expansion = E->getExpansion();
>> +  return Expansion.isMacroBodyExpansion();
>> +}
>> +
>>
>>  //===----------------------------------------------------------------------===//
>>  // Queries about the code at a SourceLocation.
>> Index: lib/Sema/SemaStmt.cpp
>> ===================================================================
>> --- lib/Sema/SemaStmt.cpp
>> +++ lib/Sema/SemaStmt.cpp
>> @@ -125,7 +125,7 @@
>>
>>    // Suppress warnings when the operator, suspicious as it may be, comes from
>>    // a macro expansion.
>> -  if (Loc.isMacroID())
>> +  if (S.SourceMgr.isMacroBodyExpansion(Loc))
>>      return false;
>>
>>    S.Diag(Loc, diag::warn_unused_comparison)
>> Index: test/Sema/unused-expr-system-header.c
>> ===================================================================
>> --- test/Sema/unused-expr-system-header.c
>> +++ test/Sema/unused-expr-system-header.c
>> @@ -3,8 +3,10 @@
>>  void f(int i1, int i2) {
>>    POSSIBLY_BAD_MACRO(5);
>>    STATEMENT_EXPR_MACRO(5);
>> -  COMMA_MACRO_1(i1 == i2, f(i1, i2)); // expected-warning {{expression result unused}}
>> +  COMMA_MACRO_1(i1 == i2, f(i1, i2)); // expected-warning {{comparison result unused}} \
>> +                                      // expected-note {{equality comparison}}
>>    COMMA_MACRO_2(i1 == i2, f(i1, i2));
>> -  COMMA_MACRO_3(i1 == i2, f(i1, i2)); // expected-warning {{expression result unused}}
>> +  COMMA_MACRO_3(i1 == i2, f(i1, i2)); // expected-warning {{comparison result unused}} \
>> +                                      // expected-note {{equality comparison}}
>>    COMMA_MACRO_4(i1 == i2, f(i1, i2));
>>  }