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

[jahanian]

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.

We are only suppressing the 'fixit' note.

> 2. The diagnostic machinery should be able to automatically suppress
> fixits that end up in macros.
> 3. Why can't we come up with the "right" source location here?

I talked to Doug. And he said that we can't. I suggest he answers that.

- fariborz

> 
> -Eli