(Beginner) Implementation of 2-dimensional array container

Question

I'd like to have a review on my 2D-array container. Any criticism is welcome! I based this on std::array implementation. Hopefully I didn't do too many bad stuff here.

#ifndef 2DARRAY
#define 2DARRAY
#include <iostream>
#include <exception>
#include <initializer_list>
#include <cassert>
#include <stdexcept>
namespace container {
 template<typename T, std::size_t rows, std::size_t columns>
 class array_2d
 {
 static_assert(rows != 0, "error: first array size cannot be 0");
 static_assert(columns != 0, "error: second array size cannot be 0");
 private:
 T m_array2d[rows][columns]{};
 
 public:
 // Aliases
 using size_type = std::size_t;
 using nested_init_list_type = std::initializer_list<std::initializer_list<T>>;
 using pointer = T*;
 using const_pointer = const T*;
 using reference = T&;
 using const_reference = const T&;
 using reverse_iterator = std::reverse_iterator<pointer>;
 using const_reverse_iterator = std::reverse_iterator<const_pointer>;
 // Avoids having to type 3 braces, allows 2 braces only
 // Note: 2 braces are always needed even for one single element
 explicit array_2d(nested_init_list_type array2d_list) {
 assert(array2d_list.size() <= rows && "Wrong number of rows [1st index] inserted");
 size_type row{ 0 };
 size_type column{ 0 };
 for (auto& currentBrace : array2d_list) {
 assert(currentBrace.size() <= columns && "Wrong number of columns [2nd index] inserted"); 
 for (auto& currentValue : currentBrace) {
 m_array2d[row][column++] = currentValue;
 }
 ++row;
 column = 0;
 } 
 }
 // Accessors (No bound checking)
 constexpr pointer operator[](size_type index) { return m_array2d[index]; }
 constexpr const_pointer operator[](size_type index) const { return m_array2d[index]; }
 constexpr reference back() noexcept { return m_array2d[rows-1][columns-1]; } 
 constexpr const_reference back() const noexcept { return m_array2d[rows-1][columns-1]; }
 constexpr pointer data() noexcept { return m_array2d[0]; }
 constexpr const_pointer data() const noexcept { return m_array2d[0]; }
 constexpr reference front() noexcept { return *(data()); }
 constexpr const_reference front() const noexcept { return *(data()); }
 // Accessors (With bound checking)
 constexpr reference at(size_type rowIndex, size_type columnIndex) 
 { return (rowIndex < rows && columnIndex < columns) ? m_array2d[rowIndex][columnIndex] : throw std::out_of_range("Error: Index out of range"); }
 constexpr const_reference at(size_type rowIndex, size_type columnIndex) const
 { return (rowIndex < rows && columnIndex < columns) ? m_array2d[rowIndex][columnIndex] : throw std::out_of_range("Error: Index out of range"); }
 // Size related
 constexpr size_type size() const noexcept { return rows * columns; }
 constexpr size_type row_size() const noexcept { return rows; }
 constexpr size_type column_size() const noexcept { return columns; }
 // Miscellaneous
 void fill(const size_type& value) noexcept {
 for (auto& element : m_array2d) 
 for (auto& current : element) 
 current = std::move(value);
 }
 void swap(array_2d& other) noexcept { std::swap(m_array2d, other.m_array2d); }
 // Iterators - Missing reverse iterators
 constexpr pointer begin() noexcept { return std::begin(m_array2d); }
 constexpr const auto begin() const noexcept { return std::begin(m_array2d); }
 constexpr const auto cbegin() const noexcept { return std::begin(m_array2d); }
 constexpr auto end() noexcept { return std::end(m_array2d); }
 constexpr const auto end() const noexcept { return std::end(m_array2d); }
 constexpr const auto cend() const noexcept { return std::end(m_array2d); }
 };
}
#endif

• L. F.: #define 2DARRAY - that's not valid.

Answer 1

Preprocessor macro identifiers can't begin with a digit, so the include guard name must be changed.

There's no reason to include <iostream> header.

A common convention for template parameters is to use PascalCase to distinguish them from members and local variables.

Here's a bug:

 for (auto& current : element) 
 current = std::move(value);

The second and subsequent iterations of the loop use the moved-from value, which is probably not what we want. We shouldn't even be able to move from a const ref anyway. Just replace with current = value; or (better!) replace the whole loop with a call to std::fill().

On the other hand, I believe we can safely move from the initializer_list in the constructor. Remember to include <utility> to declare std::move.

Well done for providing the usual type aliases. However, some are missing (e.g. iterator, value_type), and others are inconsistent with the implementation.

---

I think the real problem you have here is that we're not really clear what the element type is. I recommend making our value_type be T[columns], consistent with our begin() and end(), but also providing a view class onto all the elements as a flat array.

That would look something like

private:
 T m_array2d[rows*columns]{};
public:
 iterator begin() { return m_array2d; }
 iterator end() { return m_array2d + rows*columns; }
 // etc
 T[rows*columns]& data();
 T[rows*columns] const& data() const;

Then fill() becomes much simpler, using std::fill() on the flat view:

void fill(const size_type& value)
{
 using std::begin; // for argument-dependent lookup
 using std::end;
 auto& d = data();
 std::fill(begin(d), end(d), value);
}

• user235054: Thanks for your insightful comment. This was really on point and improved the code quite a lot!