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

[Chandler Carruth]

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.

Also, doxyments would be good.

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?

+  // 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.

+  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.

+    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...

+      if (Call->getNumArgs() == 2) {

Throughout this, could we use early-exit to simplify the code?

+        *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.

+        if (IsArithmeticOp(*Opcode)) {
+          *RHS = Call->getArg(1);
+          return true;
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20110606/57eca010/attachment.html>