From 3e4a33d4bac400f857fc165c9b119901c1c7f5e5 Mon Sep 17 00:00:00 2001
From: Christoph Hertzberg <chtz@informatik.uni-bremen.de>
Date: Mon, 8 Aug 2016 17:26:48 +0200
Subject: bug #1272: Let CommaInitializer work for more border cases (enhances
 fix of bug #1242). The unit test tests all combinations of 2x2 block-sizes
 from 0 to 3.

---
 Eigen/src/Core/CommaInitializer.h | 17 ++-------
 test/commainitializer.cpp         | 78 ++++++++++++++++++++++++++++-----------
 2 files changed, 61 insertions(+), 34 deletions(-)

diff --git a/Eigen/src/Core/CommaInitializer.h b/Eigen/src/Core/CommaInitializer.h
index 787743b8f..b39a661d0 100644
--- a/Eigen/src/Core/CommaInitializer.h
+++ b/Eigen/src/Core/CommaInitializer.h
@@ -80,12 +80,7 @@ struct CommaInitializer
   EIGEN_DEVICE_FUNC
   CommaInitializer& operator,(const DenseBase<OtherDerived>& other)
   {
-    if(other.rows()==0)
-    {
-      m_col += other.cols();
-      return *this;
-    }
-    if (m_col==m_xpr.cols())
+    if (m_col==m_xpr.cols() && (other.cols()!=0 || other.rows()!=m_currentBlockRows))
     {
       m_row+=m_currentBlockRows;
       m_col = 0;
@@ -93,15 +88,11 @@ struct CommaInitializer
       eigen_assert(m_row+m_currentBlockRows<=m_xpr.rows()
         && "Too many rows passed to comma initializer (operator<<)");
     }
-    eigen_assert((m_col<m_xpr.cols() || (m_xpr.cols()==0 && m_col==0))
+    eigen_assert((m_col + other.cols() <= m_xpr.cols())
       && "Too many coefficients passed to comma initializer (operator<<)");
     eigen_assert(m_currentBlockRows==other.rows());
-    if (OtherDerived::SizeAtCompileTime != Dynamic)
-      m_xpr.template block<OtherDerived::RowsAtCompileTime != Dynamic ? OtherDerived::RowsAtCompileTime : 1,
-                              OtherDerived::ColsAtCompileTime != Dynamic ? OtherDerived::ColsAtCompileTime : 1>
-                    (m_row, m_col) = other;
-    else
-      m_xpr.block(m_row, m_col, other.rows(), other.cols()) = other;
+    m_xpr.template block<OtherDerived::RowsAtCompileTime, OtherDerived::ColsAtCompileTime>
+                    (m_row, m_col, other.rows(), other.cols()) = other;
     m_col += other.cols();
     return *this;
   }
diff --git a/test/commainitializer.cpp b/test/commainitializer.cpp
index 86bdb040e..5ece4f6cc 100644
--- a/test/commainitializer.cpp
+++ b/test/commainitializer.cpp
@@ -9,6 +9,61 @@
 
 #include "main.h"
 
+
+template<int M1, int M2, int N1, int N2>
+void test_blocks()
+{
+  Matrix<int, M1+M2, N1+N2> m_fixed;
+  MatrixXi m_dynamic(M1+M2, N1+N2);
+
+  Matrix<int, M1, N1> mat11; mat11.setRandom();
+  Matrix<int, M1, N2> mat12; mat12.setRandom();
+  Matrix<int, M2, N1> mat21; mat21.setRandom();
+  Matrix<int, M2, N2> mat22; mat22.setRandom();
+
+  MatrixXi matx11 = mat11, matx12 = mat12, matx21 = mat21, matx22 = mat22;
+
+  // The only remaining border case is M1==M2>0 && N1==N2==0.
+  // In that case it is not possible to decide (without backtracking) if a block starts a new row or does not
+  if(M1 != M2 || M1 == 0 || N1>0 || N2>0)
+  {
+    VERIFY_IS_EQUAL((m_fixed << mat11, mat12, mat21, matx22).finished(), (m_dynamic << mat11, matx12, mat21, matx22).finished());
+    VERIFY_IS_EQUAL((m_fixed << mat12, mat11, matx21, mat22).finished(), (m_dynamic << mat12, matx11, matx21, mat22).finished());
+  }
+
+  if(N1 > 0)
+  {
+    VERIFY_RAISES_ASSERT((m_fixed << mat11, mat12, mat11, mat21, mat22));
+    VERIFY_RAISES_ASSERT((m_fixed << mat11, mat12, mat21, mat21, mat22));
+  }
+  else if(N2 > 0 || M1 != M2) // border case if both sublocks have zero columns and same number of rows
+  {
+    // allow insertion of zero-column blocks:
+    VERIFY_IS_EQUAL((m_fixed << mat11, mat12, mat11, mat11, mat21, mat21, mat22).finished(), (m_dynamic << mat12, mat22).finished());
+  }
+  if(M1 != M2)
+  {
+    VERIFY_RAISES_ASSERT((m_fixed << mat11, mat21, mat12, mat22));
+  }
+}
+
+
+template<int N>
+struct test_block_recursion
+{
+  static void run()
+  {
+    test_blocks<(N>>6)&3, (N>>4)&3, (N>>2)&3, N & 3>();
+    test_block_recursion<N-1>::run();
+  }
+};
+
+template<>
+struct test_block_recursion<-1>
+{
+  static void run() { }
+};
+
 void test_commainitializer()
 {
   Matrix3d m3;
@@ -45,25 +100,6 @@ void test_commainitializer()
   VERIFY_IS_APPROX(m3, ref);
 
 
-  // Check with empty matrices (bug #1242)
-  {
-    int const M = 0;
-    int const N1 = 2;
-    int const N2 = 1;
-
-    {
-      Matrix<double, M, N1> A1;
-      Matrix<double, M, N2> A2;
-      Matrix<double, M, N1 + N2> B;
-      B << A1, A2;
-    }
-    {
-      Matrix<double, N1, M> A1;
-      Matrix<double, N2, M> A2;
-      Matrix<double, N1 + N2, M> B;
-      B << A1,
-           A2;
-    }
-  }
-
+  // recursively test all block-sizes from 0 to 3:
+  test_block_recursion<(1<<8) - 1>();
 }
-- 
cgit v1.2.3