From 4f1c7ccfa02bfbd0da3bf9831859bb24bbdcee35 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Vladim=C3=ADr=20Vondru=C5=A1?= Date: Tue, 17 Apr 2012 23:50:11 +0200 Subject: [PATCH] Better (hopefully) way to distinguish between Matrix constructors. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Before it was one constructor using bool parameter, which is massive antipattern: Matrix4 m(false); // Huh? No Matrix4 then or what? Iẗ́'s now separated into two distinct constructors, of them one can be already declared as constexpr (hooray). The usage is as follows: Matrix4 a; // Default (identity matrix) Matrix4 b(Matrix4::Identity); // Explicitly identity matrix Matrix4 c(Matrix4::Zero); // Zero-filled matrix Also deleted constructor from one parameter, so following mistakes now cannot compile: Matrix4 d(true); // Both would set element at [0][0] to 1, Matrix4 e(Matrix3::Zero); // and other to 0 --- src/Math/Matrix.h | 33 ++++++++++++++++++++++++++------- src/Math/Matrix3.h | 17 ++++++++++++----- src/Math/Matrix4.h | 19 +++++++++++++------ src/Math/Test/MatrixTest.cpp | 10 ++++++---- 4 files changed, 57 insertions(+), 22 deletions(-) diff --git a/src/Math/Matrix.h b/src/Math/Matrix.h index 8a86bc8cd..075326f79 100644 --- a/src/Math/Matrix.h +++ b/src/Math/Matrix.h @@ -73,16 +73,35 @@ template class Matrix { return from(typename Implementation::GenerateSequence::Type(), first, next...); } + /** @brief Pass to constructor to create zero-filled matrix */ + enum ZeroType { Zero }; + + /** + * @brief Zero-filled matrix constructor + * + * Use this constructor by calling `Matrix m(Matrix::Zero);`. + */ + inline constexpr explicit Matrix(ZeroType): _data() {} + + /** @brief Pass to constructor to create identity matrix */ + enum IdentityType { Identity }; + /** * @brief Default constructor - * @param identity Create identity matrix instead of zero matrix. + * + * You can also explicitly call this constructor with + * `Matrix m(Matrix::Identity);`. */ - inline explicit Matrix(bool identity = true): _data() { + inline explicit Matrix(IdentityType = Identity): _data() { /** @todo constexpr how? */ - if(identity) for(size_t i = 0; i != size; ++i) + for(size_t i = 0; i != size; ++i) _data[size*i+i] = static_cast(1); } + #ifndef DOXYGEN_GENERATING_OUTPUT + template explicit Matrix(U) = delete; + #endif + /** * @brief Initializer-list constructor * @param first First value @@ -136,7 +155,7 @@ template class Matrix { /** @brief Multiply matrix operator */ Matrix operator*(const Matrix& other) const { - Matrix out(false); + Matrix out(Zero); for(size_t row = 0; row != size; ++row) for(size_t col = 0; col != size; ++col) @@ -164,7 +183,7 @@ template class Matrix { /** @brief Transposed matrix */ Matrix transposed() const { - Matrix out(false); + Matrix out(Zero); for(size_t row = 0; row != size; ++row) for(size_t col = 0; col != size; ++col) @@ -175,7 +194,7 @@ template class Matrix { /** @brief %Matrix without given column and row */ Matrix ij(size_t skipCol, size_t skipRow) const { - Matrix out(false); + Matrix out(Matrix::Zero); for(size_t row = 0; row != size-1; ++row) for(size_t col = 0; col != size-1; ++col) @@ -198,7 +217,7 @@ template class Matrix { * @brief Inverse matrix */ Matrix inversed() const { - Matrix out(false); + Matrix out(Zero); T _determinant = determinant(); diff --git a/src/Math/Matrix3.h b/src/Math/Matrix3.h index 1ccf21d01..a3ebb5fc1 100644 --- a/src/Math/Matrix3.h +++ b/src/Math/Matrix3.h @@ -42,14 +42,21 @@ template class Matrix3: public Matrix { return Matrix::from(first, next...); } - /** @copydoc Matrix::Matrix(bool) */ - inline constexpr explicit Matrix3(bool identity = true): Matrix{ + /** @copydoc Matrix::Matrix(ZeroType) */ + inline constexpr explicit Matrix3(typename Matrix::ZeroType): Matrix(Matrix::Zero) {} + + /** @copydoc Matrix::Matrix(IdentityType) */ + inline constexpr explicit Matrix3(typename Matrix::IdentityType = Matrix::Identity): Matrix{ /** @todo Make this in Matrix itself, after it will be constexpr */ - identity ? 1.0f : 0.0f, 0.0f, 0.0f, - 0.0f, identity ? 1.0f : 0.0f, 0.0f, - 0.0f, 0.0f, identity ? 1.0f : 0.0f + 1.0f, 0.0f, 0.0f, + 0.0f, 1.0f, 0.0f, + 0.0f, 0.0f, 1.0f } {} + #ifndef DOXYGEN_GENERATING_OUTPUT + template explicit Matrix3(U) = delete; + #endif + /** @copydoc Matrix::Matrix(T, U...) */ #ifndef DOXYGEN_GENERATING_OUTPUT template inline constexpr Matrix3(T first, U... next): Matrix(first, next...) {} diff --git a/src/Math/Matrix4.h b/src/Math/Matrix4.h index 88e4012ba..26e5e7072 100644 --- a/src/Math/Matrix4.h +++ b/src/Math/Matrix4.h @@ -110,15 +110,22 @@ template class Matrix4: public Matrix { ); } - /** @copydoc Matrix::Matrix(bool) */ - inline constexpr explicit Matrix4(bool identity = true): Matrix{ + /** @copydoc Matrix::Matrix(ZeroType) */ + inline constexpr explicit Matrix4(typename Matrix::ZeroType): Matrix(Matrix::Zero) {} + + /** @copydoc Matrix::Matrix(IdentityType) */ + inline constexpr explicit Matrix4(typename Matrix::IdentityType = Matrix::Identity): Matrix{ /** @todo Make this in Matrix itself, after it will be constexpr */ - identity ? 1.0f : 0.0f, 0.0f, 0.0f, 0.0f, - 0.0f, identity ? 1.0f : 0.0f, 0.0f, 0.0f, - 0.0f, 0.0f, identity ? 1.0f : 0.0f, 0.0f, - 0.0f, 0.0f, 0.0f, identity ? 1.0f : 0.0f + 1.0f, 0.0f, 0.0f, 0.0f, + 0.0f, 1.0f, 0.0f, 0.0f, + 0.0f, 0.0f, 1.0f, 0.0f, + 0.0f, 0.0f, 0.0f, 1.0f } {} + #ifndef DOXYGEN_GENERATING_OUTPUT + template explicit Matrix4(U) = delete; + #endif + /** @copydoc Matrix::Matrix(T, U...) */ #ifndef DOXYGEN_GENERATING_OUTPUT template inline constexpr Matrix4(T first, U... next): Matrix(first, next...) {} diff --git a/src/Math/Test/MatrixTest.cpp b/src/Math/Test/MatrixTest.cpp index c5e96c9c7..998d1b66e 100644 --- a/src/Math/Test/MatrixTest.cpp +++ b/src/Math/Test/MatrixTest.cpp @@ -65,6 +65,7 @@ void MatrixTest::constructFromVectors() { void MatrixTest::constructIdentity() { Matrix4 identity; + Matrix4 identity2(Matrix4::Identity); Matrix4 identityExpected( 1.0f, 0.0f, 0.0f, 0.0f, @@ -74,10 +75,11 @@ void MatrixTest::constructIdentity() { ); QVERIFY(identity == identityExpected); + QVERIFY(identity2 == identityExpected); } void MatrixTest::constructZero() { - Matrix4 zero(false); + Matrix4 zero(Matrix4::Zero); Matrix4 zeroExpected( 0.0f, 0.0f, 0.0f, 0.0f, @@ -90,7 +92,7 @@ void MatrixTest::constructZero() { } void MatrixTest::data() { - Matrix4 m(false); + Matrix4 m(Matrix4::Zero); Vector4 vector(4.0f, 5.0f, 6.0f, 7.0f); @@ -113,7 +115,7 @@ void MatrixTest::data() { } void MatrixTest::copy() { - Matrix4 m1(false); + Matrix4 m1(Matrix4::Zero); m1[2][3] = 1.0f; @@ -127,7 +129,7 @@ void MatrixTest::copy() { m1[3][2] = 1.0f; /* Verify the copy is the same as original */ - Matrix4 original(false); + Matrix4 original(Matrix4::Zero); original[2][3] = 1.0f; QVERIFY(m2 == original);