[PATCH] D22910: Add support for CXXOperatorCallExpr in Expr::HasSideEffects

[Richard Smith via cfe-commits]

rsmith added a comment.

If `IncludePossibleEffects` is false, the goal of `HasSideEffect` is to determine whether we think the user intended for the expression to have a side-effect. It seems reasonable to assume that any use of an assignment operator had that intent, so (since it seems like this is not adding false positives) this seems reasonable to me.


================
Comment at: lib/AST/Expr.cpp:2867-2868
@@ +2866,4 @@
+    // underlaying operator is of assignment type
+    OverloadedOperatorKind binOp = cast<CXXOperatorCallExpr>(this)->getOperator();
+    if (binOp == OO_Equal || (binOp >= OO_PlusEqual && binOp <= OO_PipeEqual)) {
+      return true;
----------------
Variable names should start with a capital letter.

================
Comment at: lib/AST/Expr.cpp:2868
@@ +2867,3 @@
+    OverloadedOperatorKind binOp = cast<CXXOperatorCallExpr>(this)->getOperator();
+    if (binOp == OO_Equal || (binOp >= OO_PlusEqual && binOp <= OO_PipeEqual)) {
+      return true;
----------------
Please don't hard-code the order of OO enumerators like this (and this isn't even correct: you missed `<<=` and `>>=`). 

Instead, consider extending `BinaryOperator::isAssignmentOp` / `BinaryOperator::getOverloadedOpcode` so you can use them for this.

================
Comment at: lib/AST/Expr.cpp:2872
@@ -2864,3 +2871,3 @@
+  }
   case CallExprClass:
   case CXXMemberCallExprClass:
----------------
Please add a comment indicating the fallthrough here is intentional.


Repository:
  rL LLVM

https://reviews.llvm.org/D22910