[PATCH] New warning -Wpessimizing-move to catch when removing calls to std::move prevent optimization

[Richard Trieu]

================
Comment at: lib/Sema/SemaInit.cpp:7346
@@ +7345,3 @@
+  QualType DestType = InitExpr->getType();
+  if (DestType->isReferenceType() || !DestType->isRecordType())
+    return;
----------------
rsmith wrote:
> The reference type check here is redundant: a record type is never a reference type.
Removed reference check.

================
Comment at: lib/Sema/SemaInit.cpp:7352-7353
@@ +7351,4 @@
+        dyn_cast<CXXConstructExpr>(InitExpr->IgnoreParens());
+    if (!CCE || !CCE->getConstructor()->isCopyOrMoveConstructor())
+      return;
+
----------------
rsmith wrote:
> This check seems to prevent your redundant_move_on_return warning from firing at the right times.
Peeled back more nodes in some cases of redundant move, including MaterializeTemporaryExpr's and additional CXXConstructorExpr's.

================
Comment at: lib/Sema/SemaInit.cpp:7377-7378
@@ +7376,4 @@
+
+    const VarDecl *VD = dyn_cast<VarDecl>(DRE->getDecl());
+    if (!VD || !VD->hasLocalStorage())
+      return;
----------------
rsmith wrote:
> You should also check that `VD` is from the current function/lambda/whatever and not an enclosing one (using `DRE->refersToEnclosingVariableOrCapture`).
Check added.

================
Comment at: lib/Sema/SemaInit.cpp:7420-7423
@@ -7361,1 +7419,6 @@
 
+  if (const Expr *E = Result.get()) {
+    CheckMoveOnConstruction(*this, E,
+                            Entity.getKind() == InitializedEntity::EK_Result);
+  }
+
----------------
rsmith wrote:
> Can you put this inside `Perform` so that all the users of `InitializationSequence` are guaranteed to reach it? There's lots of places that perform copy elision but don't use this convenience wrapper.
Moved check to InitializationSequence::Perform.

================
Comment at: test/SemaCXX/warn-pessmizing-move.cpp:38-43
@@ +37,8 @@
+B test2(A a1, B b1) {
+  // Object is different than return type so don't warn.
+  A a2;
+  return a1;
+  return a2;
+  return std::move(a1);
+  return std::move(a2);
+
----------------
rsmith wrote:
> We should give the "redundant move" warning for this case, but we won't because we didn't use a copy or move constructor.
Added this test case to the redundant move test.

http://reviews.llvm.org/D7633

EMAIL PREFERENCES
  http://reviews.llvm.org/settings/panel/emailpreferences/