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

[David Blaikie]

On Tue, Jan 27, 2015 at 3:48 PM, Hans Wennborg <hans at chromium.org> wrote:

> On Tue, Jan 27, 2015 at 3:15 PM, David Blaikie <dblaikie at gmail.com> wrote:
> >
> >
> > On Tue, Jan 27, 2015 at 9:29 AM, Hans Wennborg <hans at chromium.org>
> wrote:
> >>
> >> 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'.
> >
> >
> > Perhaps - though &&|| is trickier because it could easily be either way.
> As
> > you point out with ?: it can't actually be the other way sometimes. So
> > perhaps we could/should have the smarts to detect that case, but I'm not
> > sure where the effort tipping point is.
>
> I think it would be tricky. For
>
>   int y = ptr1 - ptr2 ? 1 : 0;
>
> we'd essentially have to tentatively build the 'ptr1 - (ptr2 ? 1 : 0)'
> expression and check if that's legal. I don't think Clang provides an
> easy way to do that :-/
>

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.

- David
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20150127/fd448ab1/attachment.html>