HiveBrain v1.2.0
Get Started
← Back to all entries
patterncppMinor

Dynamic multidimensional arrays

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
arraysmultidimensionaldynamic

Problem

My application needs Field objects, which encapsulate 1D, 2D or 3D gridded data. The number of dimensions and their size are known only at run time. Furthermore, I need the encapsulated data to be in one contiguous memory block for performance reasons.

I've based my code on the following code, which defines simple multidimensional arrays using meta-programming / variadic templates. Those arrays need to know their number of dimensions at compile-time. Therefore my object Field, being limited to 1D, 2D and 3D cases, encapsulate three of those arrays.

```
#include
#include
#include

template
class Array
{

public:
Array() = default;

template
Array(Sizes... sizes)
: linearSize_(1)
{
allocate(1, sizes...);
}

template
inline T& operator()(Ns... ns) //const
{
return data_[getIndex(1, ns...)];
}

template
inline T operator()(Ns... ns) const
{
return data_[getIndex(1, ns...)];
}

int getSize(int dim = 1) const //dim == 1,2 or 3
{
if (dim 3)
throw(std::runtime_error("invalid dimension"));
else
return dimSize_[dim - 1];
}

private:
std::vector data_;
int dimSize_[dimension];
int linearSize_;

// -------------------

// allocate memory for variable dimension array
template
void allocate(int dim, int dimsize, dimSizes... dimsizes)
{
linearSize_ *= dimsize;
dimSize_[dim - 1] = dimsize;
allocate(dim + 1, dimsizes...);
}

void allocate(int dim) // end recursion and allocate memory
{
data_.resize(linearSize_);
}

// -------------------

int computeSubSize(int dim) const
{
if (dim == dimension)
{
return 1;
}
return dimSize_[dim] * computeSubSize(dim + 1);
}

template
int getIndex(int dim, int dimsize, DimSizes... dimsizes) const
{
return dimsize * computeSubSize(dim) + getIndex(dim

Solution

Don't defer to run-time what you can check at compile-time

When I started playing with your Array, I kept getting weird segfaults until I realized that I had forgotten to set the second template parameter correctly. This is something your constructor could easily check at compile-time with no overhead.

template 
Array(Sizes... sizes) : linearSize_(1)
{
  static_assert(sizeof...(sizes) == Dimensions, "wrong number of size arguments");
  allocate(1, sizes...);
}


Likewise for operator():

template 
typename std::enable_if::type
operator()(Ns... ns)
{
  const auto idx = getIndex(1, ns...);
  return data_[idx];
}



C++14:


Your question is tagged c++11 but if you can use C++14, you can replace

typename std::enable_if::type




with

std::enable_if_t




which will do the exact same thing but look far less intimidating.

Note that I have taken the liberty to rename your dimension non-type template parameter to Dimensions. I found the singular very confusing and ambiguous with other usages of the term “dimension” in the code. I'd also recommend that you make it a habit to use CamelCase names for template parameters to distinguish them from other variables.

Avoid arbitrary restrictions

You've commented that your application currently only needs multi-dimensional arrays of at most 3 dimensions. This is fine but no reason to arbitrarily restrict your Array if it doesn't make its implementation easier. The only place where you refer to the limit is inside getSize and there the restriction shows up in the least helpful way. Your code actually allows me to create an Array but when I ask for the size of the 5th dimension, it will crash at run-time. On the other hand, asking for the size of the 3rd dimension of a 2-dimensional Array won't be caught and invoke undefined behavior.

What you should do instead is check getSize's argument against the actual number of dimensions of the Array.

int getSize(int dim = 1) const
{
  if (dim  Dimensions)
    throw(std::runtime_error("invalid dimension"));
  else
    return dimSize_[dim - 1];
}


But I have more to say about this function later…

If you really need to restrict the maximum number of dimensions of an Array, do it statically at class scope. However, instead of disallowing more than 3 dimensions, I'd rather enforce it has at least 1.

template 
class Array
{
  static_assert(Dimensions >= 1, "Array must have at least 1 dimension");

  // …
};


Use 0-based indexing

It really confused me that getSize expects 1-based indexing of dimensions. This choice also makes your implementation more complicated because now you have to internally map the indices back to 0-based indices for subscripting the dimSize_ array.

Avoid non-intuitive default parameters

Defaulting parameters to reasonable default values can help making your interfaces easier to use. However, if the default is non-obvious, it makes the interface more confusing and hides bugs. For example, I don't see why getSize should give me the size of the first dimension if I forget to specify one. Likewise, what justifies defaulting the number of dimensions of the Array to 1? If the default isn't obvious, I'd embrace the guideline that “explicit is better than implicit”.

Consider a re-design of Array::getSize

Combining some of the above thoughts, I would recommend a re-design of getSize. Zeroth, change the index to be 0-based. First, I would make the function a template that takes no arguments and a non-type template parameter for the dimension instead. Once you do this, you can statically check the dimension (against the actual number of dimensions).

template 
typename std::enable_if= 0) && (Dim ::type
getSize() const noexcept
{
  return dimSize_[Dim];
}


Now we could offer an overload that returns the total number of elements in the Array which might be useful sometimes.

int
getSize() const noexcept
{
  return static_cast(data_.size());
}


Consider using std::size_t for indices

Note the ugly static_cast in the above snippet? Some people make a point that it was a mistake to use unsigned types for array dimensions and indices but its use is now so prevalent that converting back and forth generally causes more trouble than it does good.

The linearSize_ member of Array is redundant

See the above implementation of getSize. The std::vector already knows how big it is.

Array::operator() should always return a reference

I'd be consistent with the standard library containers and always return a reference from subscripting functions (also the const overloads). If T == int or a similar cheaply copyable type, it doesn't really matter but people might also want to put std::strings or other expensive types into your Array. Since your implementation is based on std::vector and the latter always returns a reference anyway, simply passing it thorough doesn't cost you a

Code Snippets

template <typename... Sizes>
Array(Sizes... sizes) : linearSize_(1)
{
  static_assert(sizeof...(sizes) == Dimensions, "wrong number of size arguments");
  allocate(1, sizes...);
}
template <typename... Ns>
typename std::enable_if<(sizeof...(Ns) == Dimensions), T&>::type
operator()(Ns... ns)
{
  const auto idx = getIndex(1, ns...);
  return data_[idx];
}
typename std::enable_if<(sizeof...(Ns) == Dimensions), T&>::type
std::enable_if_t<(sizeof...(Ns) == Dimensions), T&>
int getSize(int dim = 1) const
{
  if (dim < 1 || dim > Dimensions)
    throw(std::runtime_error("invalid dimension"));
  else
    return dimSize_[dim - 1];
}

Context

StackExchange Code Review Q#127777, answer score: 2

Revisions (0)

No revisions yet.