bug #1746: Removed implementation of standard copy-constructor and standard copy-assign-operator from PermutationMatrix and Transpositions to allow malloc-less std::move. Added unit-test to rvalue_types

Eigen/src/Core/PermutationMatrix.h

@@ -87,17 +87,6 @@ class PermutationBase : public EigenBase<Derived>
       return derived();
     }
 
-    #ifndef EIGEN_PARSED_BY_DOXYGEN
-    /** This is a special case of the templated operator=. Its purpose is to
-      * prevent a default operator= from hiding the templated operator=.
-      */
-    Derived& operator=(const PermutationBase& other)
-    {
-      indices() = other.indices();
-      return derived();
-    }
-    #endif
-
     /** \returns the number of rows */
     inline EIGEN_DEVICE_FUNC Index rows() const { return Index(indices().size()); }
 
@@ -333,12 +322,6 @@ class PermutationMatrix : public PermutationBase<PermutationMatrix<SizeAtCompile
     inline PermutationMatrix(const PermutationBase<OtherDerived>& other)
       : m_indices(other.indices()) {}
 
-    #ifndef EIGEN_PARSED_BY_DOXYGEN
-    /** Standard copy constructor. Defined only to prevent a default copy constructor
-      * from hiding the other templated constructor */
-    inline PermutationMatrix(const PermutationMatrix& other) : m_indices(other.indices()) {}
-    #endif
-
     /** Generic constructor from expression of the indices. The indices
       * array has the meaning that the permutations sends each integer i to indices[i].
       *
@@ -373,17 +356,6 @@ class PermutationMatrix : public PermutationBase<PermutationMatrix<SizeAtCompile
       return Base::operator=(tr.derived());
     }
 
-    #ifndef EIGEN_PARSED_BY_DOXYGEN
-    /** This is a special case of the templated operator=. Its purpose is to
-      * prevent a default operator= from hiding the templated operator=.
-      */
-    PermutationMatrix& operator=(const PermutationMatrix& other)
-    {
-      m_indices = other.m_indices;
-      return *this;
-    }
-    #endif
-
     /** const version of indices(). */
     const IndicesType& indices() const { return m_indices; }
     /** \returns a reference to the stored array representing the permutation. */

Eigen/src/Core/Transpositions.h

@@ -34,17 +34,6 @@ class TranspositionsBase
       return derived();
     }
 
-    #ifndef EIGEN_PARSED_BY_DOXYGEN
-    /** This is a special case of the templated operator=. Its purpose is to
-      * prevent a default operator= from hiding the templated operator=.
-      */
-    Derived& operator=(const TranspositionsBase& other)
-    {
-      indices() = other.indices();
-      return derived();
-    }
-    #endif
-
     /** \returns the number of transpositions */
     Index size() const { return indices().size(); }
     /** \returns the number of rows of the equivalent permutation matrix */
@@ -171,12 +160,6 @@ class Transpositions : public TranspositionsBase<Transpositions<SizeAtCompileTim
     inline Transpositions(const TranspositionsBase<OtherDerived>& other)
       : m_indices(other.indices()) {}
 
-    #ifndef EIGEN_PARSED_BY_DOXYGEN
-    /** Standard copy constructor. Defined only to prevent a default copy constructor
-      * from hiding the other templated constructor */
-    inline Transpositions(const Transpositions& other) : m_indices(other.indices()) {}
-    #endif
-
     /** Generic constructor from expression of the transposition indices. */
     template<typename Other>
     explicit inline Transpositions(const MatrixBase<Other>& indices) : m_indices(indices)
@@ -189,17 +172,6 @@ class Transpositions : public TranspositionsBase<Transpositions<SizeAtCompileTim
       return Base::operator=(other);
     }
 
-    #ifndef EIGEN_PARSED_BY_DOXYGEN
-    /** This is a special case of the templated operator=. Its purpose is to
-      * prevent a default operator= from hiding the templated operator=.
-      */
-    Transpositions& operator=(const Transpositions& other)
-    {
-      m_indices = other.m_indices;
-      return *this;
-    }
-    #endif
-
     /** Constructs an uninitialized permutation matrix of given size.
       */
     inline Transpositions(Index size) : m_indices(size)
@@ -306,17 +278,6 @@ class TranspositionsWrapper
       return Base::operator=(other);
     }
 
-    #ifndef EIGEN_PARSED_BY_DOXYGEN
-    /** This is a special case of the templated operator=. Its purpose is to
-      * prevent a default operator= from hiding the templated operator=.
-      */
-    TranspositionsWrapper& operator=(const TranspositionsWrapper& other)
-    {
-      m_indices = other.m_indices;
-      return *this;
-    }
-    #endif
-
     /** const version of indices(). */
     const IndicesType& indices() const { return m_indices; }
 

test/rvalue_types.cpp

@@ -7,6 +7,8 @@
 // Public License v. 2.0. If a copy of the MPL was not distributed
 // with this file, You can obtain one at http://mozilla.org/MPL/2.0/.
 
+#define EIGEN_RUNTIME_NO_MALLOC
+
 #include "main.h"
 
 #include <Eigen/Core>
@@ -24,27 +26,65 @@ void rvalue_copyassign(const MatrixType& m)
   MatrixType tmp = m;
   UIntPtr src_address = reinterpret_cast<UIntPtr>(tmp.data());
   
+  Eigen::internal::set_is_malloc_allowed(false); // moving from an rvalue reference shall never allocate
   // move the temporary to n
   MatrixType n = std::move(tmp);
   UIntPtr dst_address = reinterpret_cast<UIntPtr>(n.data());
-
   if (MatrixType::RowsAtCompileTime==Dynamic|| MatrixType::ColsAtCompileTime==Dynamic)
   {
     // verify that we actually moved the guts
     VERIFY_IS_EQUAL(src_address, dst_address);
+    VERIFY_IS_EQUAL(tmp.size(), 0);
+    VERIFY_IS_EQUAL(reinterpret_cast<UIntPtr>(tmp.data()), UIntPtr(0));
   }
 
   // verify that the content did not change
   Scalar abs_diff = (m-n).array().abs().sum();
   VERIFY_IS_EQUAL(abs_diff, Scalar(0));
+  Eigen::internal::set_is_malloc_allowed(true);
+}
+template<typename TranspositionsType>
+void rvalue_transpositions(Index rows)
+{
+  typedef typename TranspositionsType::IndicesType PermutationVectorType;
+
+  PermutationVectorType vec;
+  randomPermutationVector(vec, rows);
+  TranspositionsType t0(vec);
+
+  Eigen::internal::set_is_malloc_allowed(false); // moving from an rvalue reference shall never allocate
+
+  UIntPtr t0_address = reinterpret_cast<UIntPtr>(t0.indices().data());
+
+  // Move constructors:
+  TranspositionsType t1 = std::move(t0);
+  UIntPtr t1_address = reinterpret_cast<UIntPtr>(t1.indices().data());
+  VERIFY_IS_EQUAL(t0_address, t1_address);
+  // t0 must be de-allocated:
+  VERIFY_IS_EQUAL(t0.size(), 0);
+  VERIFY_IS_EQUAL(reinterpret_cast<UIntPtr>(t0.indices().data()), UIntPtr(0));
+
+
+  // Move assignment:
+  t0 = std::move(t1);
+  t0_address = reinterpret_cast<UIntPtr>(t0.indices().data());
+  VERIFY_IS_EQUAL(t0_address, t1_address);
+  // t1 must be de-allocated:
+  VERIFY_IS_EQUAL(t1.size(), 0);
+  VERIFY_IS_EQUAL(reinterpret_cast<UIntPtr>(t1.indices().data()), UIntPtr(0));
+
+  Eigen::internal::set_is_malloc_allowed(true);
 }
 #else
 template <typename MatrixType>
 void rvalue_copyassign(const MatrixType&) {}
+template<typename TranspositionsType>
+void rvalue_transpositions(Index) {}
 #endif
 
 EIGEN_DECLARE_TEST(rvalue_types)
 {
+  for(int i = 0; i < g_repeat; i++) {
     CALL_SUBTEST_1(rvalue_copyassign( MatrixXf::Random(50,50).eval() ));
     CALL_SUBTEST_1(rvalue_copyassign( ArrayXXf::Random(50,50).eval() ));
 
@@ -61,4 +101,10 @@ EIGEN_DECLARE_TEST(rvalue_types)
     CALL_SUBTEST_2(rvalue_copyassign( Array<float,2,2>::Random().eval() ));
     CALL_SUBTEST_2(rvalue_copyassign( Array<float,3,3>::Random().eval() ));
     CALL_SUBTEST_2(rvalue_copyassign( Array<float,4,4>::Random().eval() ));
+  
+    CALL_SUBTEST_3((rvalue_transpositions<PermutationMatrix<Dynamic, Dynamic, int> >(internal::random<int>(1,EIGEN_TEST_MAX_SIZE))));
+    CALL_SUBTEST_3((rvalue_transpositions<PermutationMatrix<Dynamic, Dynamic, Index> >(internal::random<int>(1,EIGEN_TEST_MAX_SIZE))));
+    CALL_SUBTEST_4((rvalue_transpositions<Transpositions<Dynamic, Dynamic, int> >(internal::random<int>(1,EIGEN_TEST_MAX_SIZE))));
+    CALL_SUBTEST_4((rvalue_transpositions<Transpositions<Dynamic, Dynamic, Index> >(internal::random<int>(1,EIGEN_TEST_MAX_SIZE))));
+  }
 }