[cfe-commits] r129465 - in /cfe/trunk: lib/Sema/SemaExpr.cpp test/FixIt/no-macro-fixit.c

[Douglas Gregor]

On Apr 13, 2011, at 2:02 PM, Eli Friedman wrote:

> On Wed, Apr 13, 2011 at 1:31 PM, Fariborz Jahanian <fjahanian at apple.com> wrote:
>> Author: fjahanian
>> Date: Wed Apr 13 15:31:26 2011
>> New Revision: 129465
>> 
>> URL: http://llvm.org/viewvc/llvm-project?rev=129465&view=rev
>> Log:
>> No fixit hint for builtin expressions which are
>> defined in a macro. // rdar://9091893
>> 
>> Added:
>>    cfe/trunk/test/FixIt/no-macro-fixit.c
>> Modified:
>>    cfe/trunk/lib/Sema/SemaExpr.cpp
>> 
>> Modified: cfe/trunk/lib/Sema/SemaExpr.cpp
>> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaExpr.cpp?rev=129465&r1=129464&r2=129465&view=diff
>> ==============================================================================
>> --- cfe/trunk/lib/Sema/SemaExpr.cpp (original)
>> +++ cfe/trunk/lib/Sema/SemaExpr.cpp Wed Apr 13 15:31:26 2011
>> @@ -10070,9 +10070,6 @@
>>     return;
>>   }
>> 
>> -  SourceLocation Open = E->getSourceRange().getBegin();
>> -  SourceLocation Close = PP.getLocForEndOfToken(E->getSourceRange().getEnd());
>> -
>>   Diag(Loc, diagnostic) << E->getSourceRange();
>> 
>>   if (IsOrAssign)
>> @@ -10082,9 +10079,14 @@
>>     Diag(Loc, diag::note_condition_assign_to_comparison)
>>       << FixItHint::CreateReplacement(Loc, "==");
>> 
>> -  Diag(Loc, diag::note_condition_assign_silence)
>> -    << FixItHint::CreateInsertion(Open, "(")
>> -    << FixItHint::CreateInsertion(Close, ")");
>> +  SourceLocation Open = E->getSourceRange().getBegin();
>> +  SourceLocation Close = E->getSourceRange().getEnd();
>> +  if (!Open.isMacroID() && !Close.isMacroID()) {
>> +    SourceLocation LocForEndOfToken = PP.getLocForEndOfToken(Close);
>> +    Diag(Loc, diag::note_condition_assign_silence)
>> +      << FixItHint::CreateInsertion(Open, "(")
>> +      << FixItHint::CreateInsertion(LocForEndOfToken, ")");
>> +  }
>>  }
>> 
>>  /// \brief Redundant parentheses over an equality comparison can indicate
>> 
>> Added: cfe/trunk/test/FixIt/no-macro-fixit.c
>> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/test/FixIt/no-macro-fixit.c?rev=129465&view=auto
>> ==============================================================================
>> --- cfe/trunk/test/FixIt/no-macro-fixit.c (added)
>> +++ cfe/trunk/test/FixIt/no-macro-fixit.c Wed Apr 13 15:31:26 2011
>> @@ -0,0 +1,15 @@
>> +// RUN: %clang_cc1 -pedantic -fixit -x c %s
>> +// rdar://9091893
>> +
>> +#define va_arg(ap, type)    __builtin_va_arg(ap, type)
>> +typedef __builtin_va_list va_list;
>> +
>> +void myFunc() {
>> +    va_list values;
>> +
>> +    int value;
>> +
>> +    while (value = va_arg(values, int)) {  // expected-warning {{using the result of an assignment as a condition without parentheses}} \
>> +                                           // expected-note {{use '==' to turn this assignment into an equality comparison}}
>> +    }
>> +}
> 
> This doesn't really look like the right solution for a few reasons:
> 1. We don't really want to suppress the note, even if there isn't a fixit.

Right.

> 2. The diagnostic machinery should be able to automatically suppress
> fixits that end up in macros.

Actually, this is strange: I thought this code already existed. I wonder why the Fix-Its with macro-instantiation source locations aren't getting implicitly dropped appropriately?

> 3. Why can't we come up with the "right" source location here?


This is an interesting special case, where the source location we're looking at is in a macro instantiation but the actual fix could go into the file itself. We probably could detect this case, and make it work. Personally, I don't think it's worth the effort.

	- Doug