[cfe-commits] [Patch] Handle overloaded operators in warning about conditional operator precedence

[Hans Wennborg]

Thank you very much for the comments, Chandler!

On Mon, Jun 6, 2011 at 6:55 PM, Chandler Carruth <chandlerc at google.com> wrote:
> I have a few problems with this patch.
> First, I like breaking up parts of it into simpler code, but unfortunately
> we're duplicating a fair amount of work stripping off various parts of the
> expression. I would lift the stripping logic back into the common function
> so that its only ever done once.

I'm not sure I follow you. The stripping *is* only done once:
IsArithmeticBinaryExpr works with the condition expression, and
ExprLooksBoolean works with the right-hand side of it.

Or do you mean the code is duplicated because we could just use
Expr::IgnoreParenImpCasts()? In that case I'm with you :)

> Also, doxyments would be good.

Done. I'm not too familiar with doxygen, so please nag me if I didn't
get it right.

> A few other comments on the patch itself:
> +  // Remove implicit casts.
> +  while (ImplicitCastExpr *C = dyn_cast<ImplicitCastExpr>(E))
> +    E = C->getSubExpr();
> Do you want to leave ParenExpr's here or not? If not, use
> E->IgnoreParenImpCasts(). If so, want to add Expr::IgnoreImpCasts() and use
> that instead?

Ah, didn't know about IgnoreParenImpCasts(). Using that.

> +  // Remove call to conversion op.
> +  if (CXXMemberCallExpr *MCE = dyn_cast<CXXMemberCallExpr>(E)) {
> +    if (isa<CXXConversionDecl>(MCE->getMethodDecl()))
> +      E = MCE->getImplicitObjectArgument();
> +  }
> This also seems like a useful predicate to add to Expr... Does this handle
> both coversion operators and implicit constructors? I can't remember if the
> latter are modeled as implicit casts or ConstructorExprs off the top of my
> head.

This doesn't handle implicit constructors (they're modeled as
CXXConstructExpr), and I don't think it has to either.

Since this is the condition expression, it has to be implicitly
convertable to bool, and if I remember correctly, conversions such as
"x -> implicit constructor -> conversion op" can't happen in C++.

I've added Expr::IgnoreConversionOperator().

> +  if (CXXOperatorCallExpr* Call = dyn_cast<CXXOperatorCallExpr>(E)) {
> You don't ignore parens or imp casts from E which may have come from
> MCE->getImplicitObjectArgument() above.

Fixed.

> +    OverloadedOperatorKind OO = Call->getOperator();
> +    if (OO >= OO_Plus && OO <= OO_Arrow) {
> It would be good to comment on what this is doing. Currently it doesn't make
> a lot of sense to me, for example it includes both ! and ~. It feels like
> the num args and opcode tests below should be sufficient...

Added a comment. The purpose of the if statement is to make sure it's
safe to pass this OverloadedOperatorKind into
BinaryOperator::getOverloadedOpcode(). Not everything that takes two
arguments is a binary operator (e.g. the subscript operator).

> +      if (Call->getNumArgs() == 2) {
> Throughout this, could we use early-exit to simplify the code?

Yes, doing that.

> +        *Opcode = BinaryOperator::getOverloadedOpcode(OO);
> This is the only point at which we set one of the output parameters and can
> (potentially) return false... that seems a bit surprising.

Fixed.

> +        if (IsArithmeticOp(*Opcode)) {
> +          *RHS = Call->getArg(1);
> +          return true;
>
>

Thanks for your time!
Hans
-------------- next part --------------
A non-text attachment was scrubbed...
Name: conditional_precedence_overloaded_operators-2.patch
Type: text/x-patch
Size: 8223 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20110607/d1e5ab1b/attachment.bin>