On Tue, Jan 27, 2015 at 3:48 PM, Hans Wennborg <[email protected]> wrote:
> On Tue, Jan 27, 2015 at 3:15 PM, David Blaikie <[email protected]> wrote: > > > > > > On Tue, Jan 27, 2015 at 9:29 AM, Hans Wennborg <[email protected]> > wrote: > >> > >> On Thu, Jan 22, 2015 at 4:55 PM, Hans Wennborg <[email protected]> > wrote: > >> > On Thu, Jan 22, 2015 at 2:43 PM, David Blaikie <[email protected]> > >> > wrote: > >> >> > >> >> > >> >> On Thu, Jan 22, 2015 at 2:11 PM, Hans Wennborg <[email protected]> > 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
_______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
