Remove ExpressionRef Move Constructor
The move constructor and move =operator are not required. They make
the code more complex and prone to bugs. The few saved assignments
are all trivial and are optimized away by the compiler or our
optimizer.
In fact, there is a bug in the current move-constructor implementation
that occurs, for example, when moving Eigen matrices around.
Change-Id: I013796495bb39f3f27677111bd0aaf49e2454e20
diff --git a/include/ceres/codegen/internal/expression_ref.h b/include/ceres/codegen/internal/expression_ref.h
index 1bdc3b5..5a13d76 100644
--- a/include/ceres/codegen/internal/expression_ref.h
+++ b/include/ceres/codegen/internal/expression_ref.h
@@ -33,6 +33,7 @@
#define CERES_PUBLIC_EXPRESSION_REF_H_
#include <string>
+
#include "ceres/codegen/internal/expression.h"
#include "ceres/codegen/internal/types.h"
@@ -86,23 +87,6 @@
ExpressionRef(const ExpressionRef& other);
ExpressionRef& operator=(const ExpressionRef& other);
- // Similar to the copy assignment above, but if 'this' is uninitialized, we
- // can remove the copy and therefore eliminate one expression in the graph.
- // For example:
- // T c;
- // c = a + b;
- // will generate
- // v_2 = v_0 + v_1
- // instead of an additional assigment from the temporary 'a + b' to 'c'. In
- // C++ this concept is called "Copy Elision". This is used by the compiler to
- // eliminate copies, for example, in a function that returns an object by
- // value. We implement it ourself here, because large parts of copy elision
- // are implementation defined, which means that every compiler can do it
- // differently. More information on copy elision can be found here:
- // https://en.cppreference.com/w/cpp/language/copy_elision
- ExpressionRef(ExpressionRef&& other);
- ExpressionRef& operator=(ExpressionRef&& other);
-
// Compound operators
ExpressionRef& operator+=(const ExpressionRef& x);
ExpressionRef& operator-=(const ExpressionRef& x);
diff --git a/internal/ceres/codegen/expression_ref_test.cc b/internal/ceres/codegen/expression_ref_test.cc
index 30672ad..0a70d42 100644
--- a/internal/ceres/codegen/expression_ref_test.cc
+++ b/internal/ceres/codegen/expression_ref_test.cc
@@ -34,6 +34,7 @@
#define CERES_CODEGEN
#include "ceres/codegen/internal/expression_ref.h"
+
#include "ceres/codegen/internal/expression_graph.h"
#include "gtest/gtest.h"
@@ -112,18 +113,6 @@
EXPECT_EQ(reference, graph);
}
-TEST(ExpressionRef, MoveAssignmentCreate) {
- StartRecordingExpressions();
- T a = 2;
- T b = std::move(a);
- auto graph = StopRecordingExpressions();
- EXPECT_EQ(graph.Size(), 1);
-
- ExpressionGraph reference;
- reference.InsertBack(Expression::CreateCompileTimeConstant(2));
- EXPECT_EQ(reference, graph);
-}
-
TEST(ExpressionRef, MoveAssignment) {
StartRecordingExpressions();
T a = 1;
diff --git a/internal/ceres/expression_ref.cc b/internal/ceres/expression_ref.cc
index 52b7e0b..dda412a 100644
--- a/internal/ceres/expression_ref.cc
+++ b/internal/ceres/expression_ref.cc
@@ -76,41 +76,6 @@
return *this;
}
-ExpressionRef::ExpressionRef(ExpressionRef&& other) {
- *this = std::move(other);
-}
-
-ExpressionRef& ExpressionRef::operator=(ExpressionRef&& other) {
- // Assigning an uninitialized variable to another variable is an error.
- CHECK(other.IsInitialized()) << "Uninitialized Assignment.";
-
- if (IsInitialized()) {
- // Create assignment from other -> this
- AddExpressionToGraph(Expression::CreateAssignment(id, other.id));
- } else {
- // Special case: 'this' is uninitialized and other is an rvalue.
- // -> Implement copy elision by only setting the reference
- // This reduces the number of generated expressions roughly by a factor
- // of 2. For example, in the following statement:
- // T c = a + b;
- // The result of 'a + b' is an rvalue reference to ExpressionRef.
- // Therefore, the move constructor of 'c' is called. Since 'c' is also
- // uninitialized, this branch here is taken and the copy is removed. After
- // this function 'c' will just point to the temporary created by the 'a +
- // b' expression. This is valid, because we don't have any scoping
- // information and therefore assume global scope for all temporary
- // variables. The generated code for the single statement above, is:
- // v_2 = v_0 + v_1; // With c.id = 2
- // Without this move constructor the following two lines would be
- // generated:
- // v_2 = v_0 + v_1;
- // v_3 = v_2; // With c.id = 3
- id = other.id;
- }
- other.id = kInvalidExpressionId;
- return *this;
-}
-
// Compound operators
ExpressionRef& ExpressionRef::operator+=(const ExpressionRef& x) {
*this = *this + x;
