[PATCH] PR12463 : Warnings about nonsensical comparisons are disabled if macro expansion is involved

[Lubos Lunak]

On Tuesday 30 of July 2013, Eli Friedman wrote:
> On Sun, Jul 28, 2013 at 3:41 AM, Lubos Lunak <l.lunak at suse.cz> wrote:
> > On Monday 01 of July 2013, Eli Friedman wrote:
> >> On Sat, Jun 29, 2013 at 11:23 PM, Lubos Lunak <l.lunak at suse.cz> wrote:
> >> >  Hello,
> >> >
> >> >  could somebody please review and commit the atached patch for
> >> > pr12463? Thank
> >> > you.
> >> >
> >> > +    bool InMacro = LHS.get()->getLocStart().isMacroID() ||
> >>
> >> +                   RHS.get()->getLocStart().isMacroID();
> >>
> >> Why not just check the source location of the operator itself?  It seems
> >> like we want to diagnose "MYMACRO1 == MYMACRO2".
> >
> >  I don't know, because that's not my code. If you look at the patch, this
> > is just moved out of the first if(), in order to prevent it from applying
> > to all the cases. So maybe it's a good question, but it's not really part
> > of this issue.
>
> Okay, I'll put that aside, then.

 Actually, looking at the end of clang/test/Sema/self-comparison.c , there is 
an explicit check for not diagnosing MACRO1 == MACRO2, referencing 
rdar://problem/8435950 as the reason, whatever that is.

> +            !isa<StringLiteral>(LHSStripped) &&
> +            !isa<ObjCEncodeExpr>(LHSStripped)) {
>
> A DeclRefExpr can't be a StringLiteral.

 I guess that must be a left-over from updating the patch when it looked 
bigger than it was because of whitespace changes.

> Why should array1 == array2 and array1 <= array2 behave differently
> inside macros?
> With this patch, we won't print the right diagnostic for array1 <
> array1 expanded from a macro.

 The difference is that array1 <= array2 or array1 < array1 is nonsense, no 
matter how I look at it, I can't imagine somebody writing that intentionally. 
However array1 == array2 at least kind of  may make sense, in code that e.g. 
uses macros or #ifdef's (similarly to how some code may end up being 
if(1==1) ).

 But I have no strong opinion on this part, I can make it always or never warn 
when macros are involved if you want (or if that rdar reference above has 
anything to say about it).


 Attached patch is just for string literals, which I hope is without any 
problems or questions, and that's the original reason for my changes. I did 
the arrays part just to keep the whole piece of code consistent, so I can 
follow up with a patch for that, but I don't care much about that part.
 
-- 
 Lubos Lunak
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-warn-about-string-literal-comparisons-even-in-macros.patch
Type: text/x-diff
Size: 3444 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20131127/6d99c522/attachment.patch>