You have none standard move operators. I would expect them to be noexcep.
You have none standard move operators. I would expect them to be noexcep.
OverView
Pretty sure you use of std::construct_at() in the copy/move assignment operator is UB.
You are doing this to try and get around constexpr declarations of these operations. This compiler checking is there for your safety trying to trick the compiler by using this method may shut up the messages the compiler generates but that does not mean it is a good (or valid) idea.
CodeReview
So we have an owned RAW pointer.
T* const pv; Will pay special attention to the rule of 3/5.
Sure size_t is valid in C. But in C++ I would expect you to use std::size_t.
const size_t cols; const size_t rows; OK. You have a matrix with random data in it.
explicit constexpr Matrix2D(size_t a_rows, size_t a_cols) : pv(new T[a_rows * a_cols]), cols(a_cols), rows(a_rows) {} Not sure that is a design choice I would have made.
Might be a better choice to set the Matrix into a specific state? I see this is the next version of the constructor. So I am wondering does this provide a real useful interface to the user.
You are passing in the matrix that the class will use.
explicit constexpr Matrix2D(T* pv, size_t cols, size_t rows) : pv(pv), cols(cols), rows(rows) {} That's a perfectly valid thing to do. BUT this interface does not make it clear that you are passing ownership of that data. It would be perfectly valid for the user to do this:
int data[] = {1,2,3,4,5,6,7,8,9}; Matrix2D<in> m(data, 3, 3); I would change the interface to be clear that you are passing ownership of the array:
explicit constexpr Matrix2D(std::unique_ptr<T>&& pv, size_t cols, size_t rows) : pv(pv) , cols(cols) , rows(rows) {} constexpr Matrix2D& operator=(const Matrix2D& rhs) { ..... std::construct_at(this, newp, rhs.cols, rhs.rows); ...... return *this; } Not confortable with this.
This is the same as using placement new (but allowed in constexpr). This means we are calling the constructor on this. But this is an already fully formed object (its lifetme has started), so this is likely UB. This may work because of the simplicity of your object's members, BUT 1) I doubt it is legal (but not sure) 2) If your class becomes none trivial then this definately will not work as expected and tracking down that bug at some point in the future will be a nightmare.
You could make this valid by first calling the destructor on this to make sure the lifetime of the object was correctly ended before calling std::construct_at.
But I think it would be easier to simply swap the pointers around.
Same issue as above:
constexpr Matrix2D(Matrix2D&& rhs) noexcept : pv(rhs.pv), cols(rhs.cols), rows(rhs.rows) { std::construct_at(&rhs, nullptr, 0, 0); // set rhs to a valid, null matrix } You are calling the constructor of an object rhs whose lifetime has already started.
Just like copy assignment, move assignment can result in assignment to self. So calling delete[] pv delete's the data you may be using (if is a self assignment).
Also the std::construct_at(&rhs, nullptr, 0, 0); will then override with null the pointer on yourself (if self assignment).
Thirdly as above the issue with std::construct_at(); is the same as the last two cases.
You assume the initializer list is not empty.
constexpr Matrix2D(std::initializer_list<std::initializer_list<T>> list) : pv(new T[list.size() * (list.begin())->size()]{}), cols((list.begin())->size()), rows(list.size()) Remember: list->begin() may be equal to list->end() and thus ->size() will not be valid.
Pre-optimization:
if (rows > 0 && cols > 0) Don't think it makes the code any clearer. And in the rare case when there is no data I don't think it will make the code significantly faster.