patterncppMinor
Dynamic multidimensional arrays
Viewed 0 times
arraysmultidimensionaldynamic
Problem
My application needs
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
```
#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
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
Likewise for
C++14:
Your question is tagged c++11 but if you can use C++14, you can replace
with
which will do the exact same thing but look far less intimidating.
Note that I have taken the liberty to rename your
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
What you should do instead is check
But I have more to say about this function later…
If you really need to restrict the maximum number of dimensions of an
Use 0-based indexing
It really confused me that
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
Consider a re-design of
Combining some of the above thoughts, I would recommend a re-design of
Now we could offer an overload that
Consider using
Note the ugly
The
See the above implementation of
I'd be consistent with the standard library containers and always
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::typewith
std::enable_if_twhich 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::getSizeCombining 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 indicesNote 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 redundantSee the above implementation of
getSize. The std::vector already knows how big it is.Array::operator() should always return a referenceI'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 aCode 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&>::typestd::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.