Remove Matrix3 column accessor methods

Although reinterpret_cast happens to work with Eigen matrices, it is inherently dangerous since it relies on assumptions about the internal layout. These accessor methods were not actually used outside of tests, and for test purposes they don't offer anything that can't be achieved using zx(), zy() etc. The AccessMatrixColumnVectors test is removed entirely (since it only existed to test these methods), while the call to zCol() in ConstructTranslationMatrix is removed since the test already compares the entire list of matrix coefficients with the expected values.
codereader · Mar 1, 2022 · dfafe34 · dfafe34
1 parent 475bbef
commit dfafe34
Show file tree

Hide file tree

Showing 2 changed files with 5 additions and 60 deletions.
diff --git a/libs/math/Matrix3.h b/libs/math/Matrix3.h
@@ -12,7 +12,7 @@
  * | xx  yx  zx |
  * | xy  yy  zy |
  * | xz  yz  zz |
- * 
+ *
  */
 class Matrix3
 {
@@ -30,7 +30,7 @@ class Matrix3
     Matrix3() { }
 
     /// Construct from Eigen transform
-    explicit Matrix3(const Eigen::Projective2d& t) : 
+    explicit Matrix3(const Eigen::Projective2d& t) :
         _transform(t)
     {}
 
@@ -64,38 +64,6 @@ class Matrix3
     const double& zy() const { return _transform.matrix()(1, 2); }
     double& zz() { return _transform.matrix()(2, 2); }
     const double& zz() const { return _transform.matrix()(2, 2); }
-    /**
-     * \}
-     */
-
-     /**
-      * Return columns of the matrix as vectors.
-      * \{
-      */
-    Vector3& xCol()
-    {
-        return reinterpret_cast<Vector3&>(xx());
-    }
-    const Vector3& xCol() const
-    {
-        return reinterpret_cast<const Vector3&>(xx());
-    }
-    Vector3& yCol()
-    {
-        return reinterpret_cast<Vector3&>(yx());
-    }
-    const Vector3& yCol() const
-    {
-        return reinterpret_cast<const Vector3&>(yx());
-    }
-    Vector3& zCol()
-    {
-        return reinterpret_cast<Vector3&>(zx());
-    }
-    const Vector3& zCol() const
-    {
-        return reinterpret_cast<const Vector3&>(zx());
-    }
     /**
      * \}
      */

diff --git a/test/math/Matrix3.cpp b/test/math/Matrix3.cpp
@@ -34,8 +34,8 @@ TEST(Matrix3Test, AssignMatrixComponents)
 
 TEST(Matrix3Test, ConstructMatrixByRows)
 {
-    auto m = Matrix3::byRows(1, 2.5, 3, 
-        0.34, 51, -6, 
+    auto m = Matrix3::byRows(1, 2.5, 3,
+        0.34, 51, -6,
         7, 9, 17);
 
     // Check individual values
@@ -52,26 +52,6 @@ TEST(Matrix3Test, ConstructMatrixByRows)
     EXPECT_EQ(m.zz(), 17);
 }
 
-TEST(Matrix3Test, AccessMatrixColumnVectors)
-{
-    Matrix3 m = Matrix3::byRows(1, 4, 8, 
-        2, 9, 7,
-        11, -2, 10);
-
-    // Read column values
-    EXPECT_EQ(m.xCol(), Vector3(1, 2, 11));
-    EXPECT_EQ(m.yCol(), Vector3(4, 9, -2));
-    EXPECT_EQ(m.zCol(), Vector3(8, 7, 10));
-
-    // Write column values
-    m.xCol() = Vector3(0.1, 0.2, 0.3);
-    m.yCol() = Vector3(0.5, 0.6, 0.7);
-    m.zCol() = Vector3(0.9, 1.0, 1.1);
-    EXPECT_EQ(m, Matrix3::byColumns(0.1, 0.2, 0.3,
-        0.5, 0.6, 0.7,
-        0.9, 1.0, 1.1));
-}
-
 TEST(Matrix3Test, MatrixEquality)
 {
     Matrix3 m1 = Matrix3::byRows(1, 2, 3.5,
@@ -159,10 +139,7 @@ TEST(Matrix3Test, ConstructTranslationMatrix)
     const Vector2 translation(1.5, -2939);
     auto tm = Matrix3::getTranslation(translation);
 
-    EXPECT_EQ(tm, Matrix3::byRows(1, 0, translation.x(),
-        0, 1, translation.y(),
-        0, 0, 1));
-    EXPECT_EQ(tm.zCol(), Vector3(translation.x(), translation.y(), 1));
+    EXPECT_EQ(tm, Matrix3::byRows(1, 0, translation.x(), 0, 1, translation.y(), 0, 0, 1));
 }
 
 TEST(Matrix3Test, ConstructRotationMatrix)