<div>I have a few problems with this patch.</div><div><br></div><div>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.</div>
<div><br></div><div>Also, doxyments would be good.</div><div><br></div><div>A few other comments on the patch itself:</div><div><br></div><div><div>+  // Remove implicit casts.</div><div>+  while (ImplicitCastExpr *C = dyn_cast<ImplicitCastExpr>(E))</div>
<div>+    E = C->getSubExpr();</div></div><div><br></div><div>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?</div>
<div><br></div><div><div>+  // Remove call to conversion op.</div><div>+  if (CXXMemberCallExpr *MCE = dyn_cast<CXXMemberCallExpr>(E)) {</div><div>+    if (isa<CXXConversionDecl>(MCE->getMethodDecl()))</div>
<div>+      E = MCE->getImplicitObjectArgument();</div><div>+  }</div></div><div><br></div><div>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.</div>
<div><br></div><div><div>+  if (CXXOperatorCallExpr* Call = dyn_cast<CXXOperatorCallExpr>(E)) {</div><div><br></div><div>You don't ignore parens or imp casts from E which may have come from MCE->getImplicitObjectArgument() above.</div>
<div><br></div><div>+    OverloadedOperatorKind OO = Call->getOperator();</div><div>+    if (OO >= OO_Plus && OO <= OO_Arrow) {</div><div><br></div><div>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...</div>
<div><br></div><div>+      if (Call->getNumArgs() == 2) {</div><div><br></div><div>Throughout this, could we use early-exit to simplify the code?</div><div><br></div><div>+        *Opcode = BinaryOperator::getOverloadedOpcode(OO);</div>
<div><br></div><div>This is the only point at which we set one of the output parameters and can (potentially) return false... that seems a bit surprising.</div><div><br></div><div>+        if (IsArithmeticOp(*Opcode)) {</div>
<div>+          *RHS = Call->getArg(1);</div><div>+          return true;</div></div><div><br></div><div><br></div>