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

[jahanian]

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

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

OK. I can add another note to that effect. But I wait for Doug's feedback first.

- fj

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