<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Tue, Jan 27, 2015 at 3:48 PM, Hans Wennborg <span dir="ltr"><<a href="mailto:hans@chromium.org" target="_blank">hans@chromium.org</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="HOEnZb"><div class="h5">On Tue, Jan 27, 2015 at 3:15 PM, David Blaikie <<a href="mailto:dblaikie@gmail.com">dblaikie@gmail.com</a>> wrote:<br>
><br>
><br>
> On Tue, Jan 27, 2015 at 9:29 AM, Hans Wennborg <<a href="mailto:hans@chromium.org">hans@chromium.org</a>> wrote:<br>
>><br>
>> On Thu, Jan 22, 2015 at 4:55 PM, Hans Wennborg <<a href="mailto:hans@chromium.org">hans@chromium.org</a>> wrote:<br>
>> > On Thu, Jan 22, 2015 at 2:43 PM, David Blaikie <<a href="mailto:dblaikie@gmail.com">dblaikie@gmail.com</a>><br>
>> > wrote:<br>
>> >><br>
>> >><br>
>> >> On Thu, Jan 22, 2015 at 2:11 PM, Hans Wennborg <<a href="mailto:hans@hanshq.net">hans@hanshq.net</a>> wrote:<br>
>> >>><br>
>> >>> Author: hans<br>
>> >>> Date: Thu Jan 22 16:11:56 2015<br>
>> >>> New Revision: 226870<br>
>> >>><br>
>> >>> URL: <a href="http://llvm.org/viewvc/llvm-project?rev=226870&view=rev" target="_blank">http://llvm.org/viewvc/llvm-project?rev=226870&view=rev</a><br>
>> >>> Log:<br>
>> >>> Make the ?: precedence warning handle pointers to the left of ?<br>
>> >>><br>
>> >>> Previously, Clang would fail to warn on:<br>
>> >>><br>
>> >>> int n = x + foo ? 1 : 2;<br>
>> >>><br>
>> >>> when foo is a pointer.<br>
>> >>><br>
>> >>> Modified:<br>
>> >>> cfe/trunk/lib/Sema/SemaExpr.cpp<br>
>> >>> cfe/trunk/test/Sema/parentheses.c<br>
>> >>><br>
>> >>> Modified: cfe/trunk/lib/Sema/SemaExpr.cpp<br>
>> >>> URL:<br>
>> >>><br>
>> >>> <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaExpr.cpp?rev=226870&r1=226869&r2=226870&view=diff" target="_blank">http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaExpr.cpp?rev=226870&r1=226869&r2=226870&view=diff</a><br>
>> >>><br>
>> >>><br>
>> >>> ==============================================================================<br>
>> >>> --- cfe/trunk/lib/Sema/SemaExpr.cpp (original)<br>
>> >>> +++ cfe/trunk/lib/Sema/SemaExpr.cpp Thu Jan 22 16:11:56 2015<br>
>> >>> @@ -6129,6 +6129,8 @@ static bool ExprLooksBoolean(Expr *E) {<br>
>> >>> return IsLogicOp(OP->getOpcode());<br>
>> >>> if (UnaryOperator *OP = dyn_cast<UnaryOperator>(E))<br>
>> >>> return OP->getOpcode() == UO_LNot;<br>
>> >>> + if (E->getType()->isPointerType())<br>
>> >><br>
>> >><br>
>> >> Could we generalize this a bit further, somehow? (I haven't looked at<br>
>> >> the<br>
>> >> code in question, but it sounds like this should use some more general<br>
>> >> tool<br>
>> >> of "try to apply contextual conversion to bool" so that it matches the<br>
>> >> actual semantic situation we're interested in here)<br>
>> ><br>
>> > It's tricky, because we don't really want to match the actual<br>
>> > semantics, we want to figure out if the intention was to use 'foo' as<br>
>> > a conditional expression. That's what 'ExprLooksBoolean' does, and<br>
>> > it's erring on the side of caution.<br>
>> ><br>
>> > For example, we don't want to warn if 'foo' is int, even if that could<br>
>> > be used as a conditional expression. But we do want to warn if 'foo'<br>
>> > is 'a==b', even in C where the type of that expression is int.<br>
>> ><br>
>> > Having said that, I now realised I might have made the warning a<br>
>> > little too broad.. we want to warn here:<br>
>> ><br>
>> > int x = x + ptr ? 1 : 0;<br>
>> ><br>
>> > because 'x + ptr' seems like a pretty unlikely condition expression.<br>
>> ><br>
>> > But we don't want to warn here:<br>
>> ><br>
>> > int y = ptr1 - ptr2 ? 1 : 0;<br>
>> ><br>
>> > I'll think about that.<br>
>><br>
>> The last example actually showed up in code yesterday:<br>
>><br>
>> <a href="http://git.savannah.gnu.org/cgit/emacs.git/tree/src/emacs.c?id=emacs-24.4#n2307" target="_blank">http://git.savannah.gnu.org/cgit/emacs.git/tree/src/emacs.c?id=emacs-24.4#n2307</a><br>
>><br>
>> The reason I said I didn't want to warn here is because the expression<br>
>> can only be interpreted one way. For example,<br>
>><br>
>> int y = ptr1 - (ptr2 ? 1 : 0);<br>
>><br>
>> would not compile because the type has changed.<br>
>><br>
>> On the other hand, parentheses would certainly make it more readable,<br>
>> and that is what the warning suggests:<br>
>><br>
>> int y = (ptr1 - ptr2) ? 1 : 0;<br>
>><br>
>> So I'm now thinking maybe warning here is the right thing, just as<br>
>> -Wparentheses warns about 'a && b || c'.<br>
><br>
><br>
> Perhaps - though &&|| is trickier because it could easily be either way. As<br>
> you point out with ?: it can't actually be the other way sometimes. So<br>
> perhaps we could/should have the smarts to detect that case, but I'm not<br>
> sure where the effort tipping point is.<br>
<br>
</div></div>I think it would be tricky. For<br>
<span class=""><br>
int y = ptr1 - ptr2 ? 1 : 0;<br>
<br>
</span>we'd essentially have to tentatively build the 'ptr1 - (ptr2 ? 1 : 0)'<br>
expression and check if that's legal. I don't think Clang provides an<br>
easy way to do that :-/<br></blockquote><div><br>Yeah, I was thinking something like the delayed typo correction work, leveraging SFINAE-esque functionality. But I'm not sure how it'd work out/whether it'd be viable.<br><br>- David<br> </div></div><br></div></div>