patterncppMinor
C++ Template Matrix Class - WIP
Viewed 0 times
wipmatrixtemplateclass
Problem
I've created simple matrix class in c++ and I would like to know what you think about the code. I've left the implementation in header file for simplicity for now.
The class is not finished yet, it misses a lot of utilities, but wanted to get review of the basics.
```
#pragma once
#include
#include
#include
#include
#include
template
class Matrix
{
public:
// ** BEGIN OF CONSTRUCTION / DESTRUCTION ** //
explicit Matrix(const size_t rowCount, const size_t columnCount, const bool initWithNull = true) :
_rowCount{ rowCount },
_columnCount{ columnCount }
{
if (_rowCount == 0)
{
throw Matrix_WrongRowCount{};
}
if (_columnCount == 0)
{
throw Matrix_WrongColumnCount{};
}
_data.resize(_rowCount * _columnCount);
if (initWithNull)
{
auto it{ begin() };
while (it != end())
{
*it++ = T{};
}
}
}
// Todo, other overloads of constructor, creating vector of vectors is not effective (new called for each row + 1 if the vector didnt exist yet)
explicit Matrix(const std::vector>& arr) :
_rowCount{ arr.size() },
_columnCount{ 0 },
_data{ std::vector{} }
{
if (arr.size() == 0)
{
throw Matrix_ArrayIsEmpty{};
}
_columnCount = arr[0].size();
const size_t dataCount{ _rowCount * _columnCount };
for (const auto& row : arr)
{
if (row.size() != _columnCount)
{
throw Matrix_ArrayIsJagged{};
}
}
_data.resize(dataCount);
for (auto arrIt{ arr.begin() }, size_t row{ 0 }; arrIt != arr.end(); ++arrIt, ++row)
{
std::copy(arrIt->begin(), arrIt->end(), begin() + row * _columnCount);
}
}
Matrix(const Matrix& other) :
_rowCount{ other._rowCount },
The class is not finished yet, it misses a lot of utilities, but wanted to get review of the basics.
```
#pragma once
#include
#include
#include
#include
#include
template
class Matrix
{
public:
// ** BEGIN OF CONSTRUCTION / DESTRUCTION ** //
explicit Matrix(const size_t rowCount, const size_t columnCount, const bool initWithNull = true) :
_rowCount{ rowCount },
_columnCount{ columnCount }
{
if (_rowCount == 0)
{
throw Matrix_WrongRowCount{};
}
if (_columnCount == 0)
{
throw Matrix_WrongColumnCount{};
}
_data.resize(_rowCount * _columnCount);
if (initWithNull)
{
auto it{ begin() };
while (it != end())
{
*it++ = T{};
}
}
}
// Todo, other overloads of constructor, creating vector of vectors is not effective (new called for each row + 1 if the vector didnt exist yet)
explicit Matrix(const std::vector>& arr) :
_rowCount{ arr.size() },
_columnCount{ 0 },
_data{ std::vector{} }
{
if (arr.size() == 0)
{
throw Matrix_ArrayIsEmpty{};
}
_columnCount = arr[0].size();
const size_t dataCount{ _rowCount * _columnCount };
for (const auto& row : arr)
{
if (row.size() != _columnCount)
{
throw Matrix_ArrayIsJagged{};
}
}
_data.resize(dataCount);
for (auto arrIt{ arr.begin() }, size_t row{ 0 }; arrIt != arr.end(); ++arrIt, ++row)
{
std::copy(arrIt->begin(), arrIt->end(), begin() + row * _columnCount);
}
}
Matrix(const Matrix& other) :
_rowCount{ other._rowCount },
Solution
Your assignment operators are not exception safe and thus you don't provide the strong exception guarantee.
Personally I would rewrite this to use the Copy and Swap Idiom. That covers all your bases and is easier to write.
The move assignment works. But it is much simpler to write as a swap. Then you don't even need to check for self assignment.
Also note move semantics should be marked as
I would write them like this:
Pass by Value does not need to be const
Matrix& operator=(const Matrix& other)
{
// Check for self assignment is a pessimization of the normal case.
// Remember that self assignment is exceptionally rare, basically never
// happens. BUT you should still handle the situation.
if (this != &other)
{
// You delete your data before you know you can guarantee
// the assignment works. You should not delete this data
// until you know the assignment has worked.
_rowCount = _columnCount = 0;
_data.empty();
_data.resize(other._data.size());
// If T is an integer/float then this will never fail.
//
// But T can be anything so you can't guarantee this will
// work. So you can't delete your old data until you
// know the copy has worked.
//
// Now if you can lock this down so T is only int/float etc
// this is totally fine.
std::copy(other.begin(), other.end(), _data.begin());
_rowCount = other._rowCount;
_columnCount = other._columnCount;
}
return *this;
}Personally I would rewrite this to use the Copy and Swap Idiom. That covers all your bases and is easier to write.
The move assignment works. But it is much simpler to write as a swap. Then you don't even need to check for self assignment.
Matrix& operator=(Matrix&& other)
{
if (this != &other)
{
_rowCount = other._rowCount;
_columnCount = other._columnCount;
_data = std::move(other._data);
other._rowCount = other._columnCount = 0;
other._data = std::vector{};
}
return *this;
}Also note move semantics should be marked as
noexcept. This allows optimizations when you put your matrix into a standard container. Note: Containers will only use move semantics for their members if the member has move semantics that are noexcept. This is because the standard tries very hard to provide the strong exception guarantee.I would write them like this:
Matrix& operator=(Matrix const& other)
{
Matrix copy(other);
copy.swap(*this);
return *this;
}
Matrix& operator=(Matrix&& other) noexcept
{
other.swap(*this);
return *this;
}Pass by Value does not need to be const
const T& operator()(const size_t rowIndex, const size_t columnIndex) const { return _data[index(rowIndex, columnIndex)]; }
^^^^^ Does not buy you anything.Code Snippets
Matrix& operator=(const Matrix& other)
{
// Check for self assignment is a pessimization of the normal case.
// Remember that self assignment is exceptionally rare, basically never
// happens. BUT you should still handle the situation.
if (this != &other)
{
// You delete your data before you know you can guarantee
// the assignment works. You should not delete this data
// until you know the assignment has worked.
_rowCount = _columnCount = 0;
_data.empty();
_data.resize(other._data.size());
// If T is an integer/float then this will never fail.
//
// But T can be anything so you can't guarantee this will
// work. So you can't delete your old data until you
// know the copy has worked.
//
// Now if you can lock this down so T is only int/float etc
// this is totally fine.
std::copy(other.begin(), other.end(), _data.begin());
_rowCount = other._rowCount;
_columnCount = other._columnCount;
}
return *this;
}Matrix& operator=(Matrix&& other)
{
if (this != &other)
{
_rowCount = other._rowCount;
_columnCount = other._columnCount;
_data = std::move(other._data);
other._rowCount = other._columnCount = 0;
other._data = std::vector<T>{};
}
return *this;
}Matrix& operator=(Matrix const& other)
{
Matrix copy(other);
copy.swap(*this);
return *this;
}
Matrix& operator=(Matrix&& other) noexcept
{
other.swap(*this);
return *this;
}const T& operator()(const size_t rowIndex, const size_t columnIndex) const { return _data[index(rowIndex, columnIndex)]; }
^^^^^ Does not buy you anything.Context
StackExchange Code Review Q#159937, answer score: 3
Revisions (0)
No revisions yet.