r226870 - Make the ?: precedence warning handle pointers to the left of ?

[Hans Wennborg]

On Thu, Jan 22, 2015 at 4:55 PM, Hans Wennborg <hans at chromium.org> wrote:
> On Thu, Jan 22, 2015 at 2:43 PM, David Blaikie <dblaikie at gmail.com> wrote:
>>
>>
>> On Thu, Jan 22, 2015 at 2:11 PM, Hans Wennborg <hans at hanshq.net> wrote:
>>>
>>> Author: hans
>>> Date: Thu Jan 22 16:11:56 2015
>>> New Revision: 226870
>>>
>>> URL: http://llvm.org/viewvc/llvm-project?rev=226870&view=rev
>>> Log:
>>> Make the ?: precedence warning handle pointers to the left of ?
>>>
>>> Previously, Clang would fail to warn on:
>>>
>>>   int n = x + foo ? 1 : 2;
>>>
>>> when foo is a pointer.
>>>
>>> Modified:
>>>     cfe/trunk/lib/Sema/SemaExpr.cpp
>>>     cfe/trunk/test/Sema/parentheses.c
>>>
>>> Modified: cfe/trunk/lib/Sema/SemaExpr.cpp
>>> URL:
>>> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaExpr.cpp?rev=226870&r1=226869&r2=226870&view=diff
>>>
>>> ==============================================================================
>>> --- cfe/trunk/lib/Sema/SemaExpr.cpp (original)
>>> +++ cfe/trunk/lib/Sema/SemaExpr.cpp Thu Jan 22 16:11:56 2015
>>> @@ -6129,6 +6129,8 @@ static bool ExprLooksBoolean(Expr *E) {
>>>      return IsLogicOp(OP->getOpcode());
>>>    if (UnaryOperator *OP = dyn_cast<UnaryOperator>(E))
>>>      return OP->getOpcode() == UO_LNot;
>>> +  if (E->getType()->isPointerType())
>>
>>
>> Could we generalize this a bit further, somehow? (I haven't looked at the
>> code in question, but it sounds like this should use some more general tool
>> of "try to apply contextual conversion to bool" so that it matches the
>> actual semantic situation we're interested in here)
>
> It's tricky, because we don't really want to match the actual
> semantics, we want to figure out if the intention was to use 'foo' as
> a conditional expression. That's what 'ExprLooksBoolean' does, and
> it's erring on the side of caution.
>
> For example, we don't want to warn if 'foo' is int, even if that could
> be used as a conditional expression. But we do want to warn if 'foo'
> is 'a==b', even in C where the type of that expression is int.
>
> Having said that, I now realised I might have made the warning a
> little too broad.. we want to warn here:
>
>   int x = x + ptr ? 1 : 0;
>
> because 'x + ptr' seems like a pretty unlikely condition expression.
>
> But we don't want to warn here:
>
>   int y = ptr1 - ptr2 ? 1 : 0;
>
> I'll think about that.

The last example actually showed up in code yesterday:
http://git.savannah.gnu.org/cgit/emacs.git/tree/src/emacs.c?id=emacs-24.4#n2307

The reason I said I didn't want to warn here is because the expression
can only be interpreted one way. For example,

int y = ptr1 - (ptr2 ? 1 : 0);

would not compile because the type has changed.

On the other hand, parentheses would certainly make it more readable,
and that is what the warning suggests:

int y = (ptr1 - ptr2) ? 1 : 0;

So I'm now thinking maybe warning here is the right thing, just as
-Wparentheses warns about 'a && b || c'.

 - Hans