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

[Eli Friedman]

On Wed, Apr 13, 2011 at 2:07 PM, jahanian <fjahanian at apple.com> wrote:
>
> 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.

The user might not be aware the warning can be suppressed with
parentheses, and this fix completely hides the note, not only the
fixit.

-Eli

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