bug #903: clean swap API regarding extra enable_if parameters, and add failtests for swap

9 files changed, 61 insertions, 31 deletions

diff --git a/Eigen/src/Core/Array.h b/Eigen/src/Core/Array.h
index eaee8847b..337086615 100644
--- a/Eigen/src/Core/Array.h
+++ b/Eigen/src/Core/Array.h
@@ -258,13 +258,6 @@ class Array
       *this = other;
     }
 
-    /** Override MatrixBase::swap() since for dynamic-sized matrices of same type it is enough to swap the
-      * data pointers.
-      */
-    template<typename OtherDerived>
-    void swap(ArrayBase<OtherDerived> const & other)
-    { this->_swap(other.derived()); }
-
     EIGEN_DEVICE_FUNC inline Index innerStride() const { return 1; }
     EIGEN_DEVICE_FUNC inline Index outerStride() const { return this->innerSize(); }
 
diff --git a/Eigen/src/Core/DenseBase.h b/Eigen/src/Core/DenseBase.h
index 14c1902e4..e81b58481 100644
--- a/Eigen/src/Core/DenseBase.h
+++ b/Eigen/src/Core/DenseBase.h
@@ -167,7 +167,7 @@ template<typename Derived> class DenseBase
       OuterStrideAtCompileTime = internal::outer_stride_at_compile_time<Derived>::ret
     };
 
-    enum { ThisConstantIsPrivateInPlainObjectBase };
+    enum { IsPlainObjectBase = 0 };
 
     /** \returns the number of nonzero coefficients which is in practice the number
       * of stored coefficients. */
@@ -380,9 +380,9 @@ template<typename Derived> class DenseBase
       */
     template<typename OtherDerived>
     EIGEN_DEVICE_FUNC
-    void swap(const DenseBase<OtherDerived>& other,
-              int = OtherDerived::ThisConstantIsPrivateInPlainObjectBase)
+    void swap(const DenseBase<OtherDerived>& other)
     {
+      EIGEN_STATIC_ASSERT(!OtherDerived::IsPlainObjectBase,THIS_EXPRESSION_IS_NOT_A_LVALUE__IT_IS_READ_ONLY);
       eigen_assert(rows()==other.rows() && cols()==other.cols());
       call_assignment(derived(), other.const_cast_derived(), internal::swap_assign_op<Scalar>());
     }
diff --git a/Eigen/src/Core/Matrix.h b/Eigen/src/Core/Matrix.h
index bb6c75387..0b3d90786 100644
--- a/Eigen/src/Core/Matrix.h
+++ b/Eigen/src/Core/Matrix.h
@@ -360,15 +360,6 @@ class Matrix
       *this = other;
     }
 
-    /** \internal
-      * \brief Override MatrixBase::swap() since for dynamic-sized matrices
-      * of same type it is enough to swap the data pointers.
-      */
-    template<typename OtherDerived>
-    EIGEN_DEVICE_FUNC
-    void swap(MatrixBase<OtherDerived> const & other)
-    { this->_swap(other.derived()); }
-
     EIGEN_DEVICE_FUNC inline Index innerStride() const { return 1; }
     EIGEN_DEVICE_FUNC inline Index outerStride() const { return this->innerSize(); }
 
diff --git a/Eigen/src/Core/PlainObjectBase.h b/Eigen/src/Core/PlainObjectBase.h
index 58fe0f6e0..06e326a05 100644
--- a/Eigen/src/Core/PlainObjectBase.h
+++ b/Eigen/src/Core/PlainObjectBase.h
@@ -795,23 +795,33 @@ class PlainObjectBase : public internal::dense_xpr_base<Derived>::type
     {
       Base::setConstant(val0);
     }
-
+    
     template<typename MatrixTypeA, typename MatrixTypeB, bool SwapPointers>
     friend struct internal::matrix_swap_impl;
 
-    /** \internal generic implementation of swap for dense storage since for dynamic-sized matrices of same type it is enough to swap the
-      * data pointers.
+  public:
+    
+#ifndef EIGEN_PARSED_BY_DOXYGEN
+    /** \internal
+      * \brief Override DenseBase::swap() since for dynamic-sized matrices
+      * of same type it is enough to swap the data pointers.
       */
     template<typename OtherDerived>
     EIGEN_DEVICE_FUNC
-    void _swap(DenseBase<OtherDerived> const & other)
+    void swap(DenseBase<OtherDerived> & other)
     {
       enum { SwapPointers = internal::is_same<Derived, OtherDerived>::value && Base::SizeAtCompileTime==Dynamic };
-      internal::matrix_swap_impl<Derived, OtherDerived, bool(SwapPointers)>::run(this->derived(), other.const_cast_derived());
+      internal::matrix_swap_impl<Derived, OtherDerived, bool(SwapPointers)>::run(this->derived(), other.derived());
     }
-
-  public:
-#ifndef EIGEN_PARSED_BY_DOXYGEN
+    
+    /** \internal
+      * \brief const version forwarded to DenseBase::swap
+      */
+    template<typename OtherDerived>
+    EIGEN_DEVICE_FUNC
+    void swap(DenseBase<OtherDerived> const & other)
+    { Base::swap(other.derived()); }
+    
     EIGEN_DEVICE_FUNC 
     static EIGEN_STRONG_INLINE void _check_template_params()
     {
@@ -826,10 +836,9 @@ class PlainObjectBase : public internal::dense_xpr_base<Derived>::type
                         && (Options & (DontAlign|RowMajor)) == Options),
         INVALID_MATRIX_TEMPLATE_PARAMETERS)
     }
-#endif
 
-private:
-    enum { ThisConstantIsPrivateInPlainObjectBase };
+    enum { IsPlainObjectBase = 1 };
+#endif
 };
 
 namespace internal {
diff --git a/Eigen/src/Core/Ref.h b/Eigen/src/Core/Ref.h
index bc7ce9a14..6e6adbd31 100644
--- a/Eigen/src/Core/Ref.h
+++ b/Eigen/src/Core/Ref.h
@@ -208,7 +208,7 @@ template<typename PlainObjectType, int Options, typename StrideType> class Ref
     {
       EIGEN_STATIC_ASSERT(bool(internal::is_lvalue<Derived>::value), THIS_EXPRESSION_IS_NOT_A_LVALUE__IT_IS_READ_ONLY);
       EIGEN_STATIC_ASSERT(bool(Traits::template match<Derived>::MatchAtCompileTime), STORAGE_LAYOUT_DOES_NOT_MATCH);
-      enum { THIS_EXPRESSION_IS_NOT_A_LVALUE__IT_IS_READ_ONLY = Derived::ThisConstantIsPrivateInPlainObjectBase};
+      EIGEN_STATIC_ASSERT(!Derived::IsPlainObjectBase,THIS_EXPRESSION_IS_NOT_A_LVALUE__IT_IS_READ_ONLY);
       Base::construct(expr.const_cast_derived());
     }
 
diff --git a/failtest/CMakeLists.txt b/failtest/CMakeLists.txt
index 5343dc152..d2fea7bdc 100644
--- a/failtest/CMakeLists.txt
+++ b/failtest/CMakeLists.txt
@@ -38,6 +38,9 @@ ei_add_failtest("ref_3")
 ei_add_failtest("ref_4")
 ei_add_failtest("ref_5")
 
+ei_add_failtest("swap_1")
+ei_add_failtest("swap_2")
+
 if (EIGEN_FAILTEST_FAILURE_COUNT)
   message(FATAL_ERROR
           "${EIGEN_FAILTEST_FAILURE_COUNT} out of ${EIGEN_FAILTEST_COUNT} failtests FAILED. "
diff --git a/failtest/swap_1.cpp b/failtest/swap_1.cpp
new file mode 100644
index 000000000..106379720
--- /dev/null
+++ b/failtest/swap_1.cpp
@@ -0,0 +1,14 @@
+#include "../Eigen/Core"
+
+using namespace Eigen;
+
+int main()
+{
+  VectorXf a(10), b(10);
+#ifdef EIGEN_SHOULD_FAIL_TO_BUILD
+  const DenseBase<VectorXf> &ac(a);
+#else
+  DenseBase<VectorXf> &ac(a);
+#endif
+  b.swap(ac);
+}
diff --git a/failtest/swap_2.cpp b/failtest/swap_2.cpp
new file mode 100644
index 000000000..c130ba6e4
--- /dev/null
+++ b/failtest/swap_2.cpp
@@ -0,0 +1,14 @@
+#include "../Eigen/Core"
+
+using namespace Eigen;
+
+int main()
+{
+  VectorXf a(10), b(10);
+  VectorXf const &ac(a);
+#ifdef EIGEN_SHOULD_FAIL_TO_BUILD
+  b.swap(ac);
+#else
+  b.swap(ac.const_cast_derived());
+#endif
+}
\ No newline at end of file
diff --git a/test/swap.cpp b/test/swap.cpp
index 36b353148..dc3610085 100644
--- a/test/swap.cpp
+++ b/test/swap.cpp
@@ -41,9 +41,15 @@ template<typename MatrixType> void swap(const MatrixType& m)
   OtherMatrixType m3_copy = m3;
   
   // test swapping 2 matrices of same type
+  Scalar *d1=m1.data(), *d2=m2.data();
   m1.swap(m2);
   VERIFY_IS_APPROX(m1,m2_copy);
   VERIFY_IS_APPROX(m2,m1_copy);
+  if(MatrixType::SizeAtCompileTime==Dynamic)
+  {
+    VERIFY(m1.data()==d2);
+    VERIFY(m2.data()==d1);
+  }
   m1 = m1_copy;
   m2 = m2_copy;