Last week, I posted this question in order to get some comments and critiques on a simple implementation of a multidimensional array class -- hyper_array::array
which was inspired by orca_array
.
I managed to fix a few issues that were mentioned by the comments and added some improvements.
Goals
I still have the same goals as the ones mentioned in my previous post. In particular:
- Self-contained (no dependency to external libraries), single-header implementation
- Clarity and "readability" of the implementation (I expect the users to have a look at the header file and get a clear idea about what they can do with the class)
- Clean API "that makes sense" to the user
- As much compile-time computation/input validation as possible (template-metaprogramming,
constexpr
?) - Maximum efficiency while remaining written in standard, portable C++11,
- Allow inclusion in STL containers
- Simple, yet may actually prove useful to use
Features
Currently, the class has the following features:
- Compile-time specification of the element type and number of dimensions,
- (Copy|Move) (constructor|assignment),
- Same iterators as
std::array
. They iterate over the underlying 1D data array, - A
clone()
method, - The ability to (access|assign) cells using either an index "tuple" (think variadic templates, not
std::tuple
) withoperator()
orat()
or a "linear" index (actual index in the underlying 1D data array) withoperator[]
, typedef
s (templatedusing
s, actually) for dimensions 1 to 9 (it's totally arbitrary.orca_array
defines a maximum of 7),(削除) Pretty (削除ここまで)Printing tostd::ostream
with an overloadedoperator<<
Concerns
- Performance. I think that compile-time computations should be possible to add, but I'm not sure,
- Cleanliness and usefulness of the API,
- Consistency of the naming convention and the code formatting,
- This revision introduced simple iterators. I'm also thinking of another type of iterators, as suggested by a previous comment, which would iterate along a given dimension. In this case I would need to design something like a "hyper array view" (because iterating over a given dimension of an N-dimension array should return (N-1)-dimension arrays). Not sure how to do it efficiently though.
hyper_array.hpp
#pragma once
// make sure that -std=c++11 or -std=c++14 ... is enabled in case of clang and gcc
#if (__cplusplus < 201103L) // C++11 ?
#error "hyper_array requires a C++11-capable compiler"
#endif
// <editor-fold desc="Configuration">
#ifndef HYPER_ARRAY_CONFIG_Overload_Stream_Operator
/// Enables/disables `operator<<()` overloading for hyper_array::array
#define HYPER_ARRAY_CONFIG_Overload_Stream_Operator 1
#endif
// </editor-fold>
// <editor-fold desc="Includes">
// std
//#include <algorithm> // during dev. replaced by compile-time equivalents in hyper_array::internal
#include <array> // std::array for hyper_array::array::dimensionLengths and indexCoeffs
#include <memory> // unique_ptr for hyper_array::array::_dataOwner
#if HYPER_ARRAY_CONFIG_Overload_Stream_Operator
#include <ostream> // ostream for the overloaded operator<<()
#endif
#include <sstream> // stringstream in hyper_array::array::validateIndexRanges()
#include <type_traits> // template metaprogramming stuff in hyper_array::internal
// </editor-fold>
/// The hyper_array lib's namespace
namespace hyper_array
{
// <editor-fold defaultstate="collapsed" desc="Internal Helper Blocks">
/// Helper functions for hyper_array::array's implementation
/// @note Everything here is subject to change and must NOT be used by user code
namespace internal
{
/// shorthand for the enable_if syntax
/// @see http://en.cppreference.com/w/cpp/types/enable_if#Helper_types
template <bool b, typename T = void>
using enable_if_t = typename std::enable_if<b, T>::type;
/// building block for a neat trick for checking multiple types against a given trait
template <bool...>
struct bool_pack
{};
/// Neat trick for checking multiple types against a given trait
/// https://codereview.stackexchange.com/a/107903/86688
template <bool... bs>
using are_all_same = std::is_same<bool_pack<true, bs...>,
bool_pack<bs..., true>>;
/// Checks that all the template arguments are integral types
/// by removing any reference then using `std::is_integral`
template <typename... Ts>
using are_integral = are_all_same<
std::is_integral<
typename std::remove_reference<Ts>::type
>::value...
>;
/// Compile-time sum
template <typename T>
constexpr T ct_plus(const T x, const T y)
{
return x + y;
}
/// Compile-time product
template <typename T>
constexpr T ct_prod(const T x, const T y)
{
return x * y;
}
/// Compile-time equivalent to `std::accumulate()`
template
<
typename T, ///< result type
std::size_t N, ///< length of the array
typename O ///< type of the binary operation
>
constexpr
T ct_accumulate(const ::std::array<T, N>& arr, ///< accumulate from this array
const size_t first, ///< starting from this position
const size_t length, ///< accumulate this number of elements
const T initialValue, ///< let this be the accumulator's initial value
const O& op ///< use this binary operation
)
{
// https://stackoverflow.com/a/33158265/865719
return (first < (first + length))
? op(arr[first],
ct_accumulate(arr,
first + 1,
length - 1,
initialValue,
op))
: initialValue;
}
/// Compile-time equivalent to `std::inner_product()`
template
<
typename T, ///< the result type
typename T_1, ///< first array's type
size_t N_1, ///< length of the first array
typename T_2, ///< second array's type
size_t N_2, ///< length of the second array
typename O_SUM, ///< summation operation's type
typename O_PROD ///< multiplication operation's type
>
constexpr
T ct_inner_product(const ::std::array<T_1, N_1>& arr_1, ///< calc the inner product of this array
const size_t first_1, ///< from this position
const ::std::array<T_2, N_2>& arr_2, ///< with this array
const size_t first_2, ///< from this position
const size_t length, ///< using this many elements from both arrays
const T initialValue, ///< let this be the summation's initial value
const O_SUM& op_sum, ///< use this as the summation operator
const O_PROD& op_prod ///< use this as the multiplication operator
)
{
// same logic as `ct_accumulate()`
return (first_1 < (first_1 + length))
? op_sum(op_prod(arr_1[first_1],
arr_2[first_2]),
ct_inner_product(arr_1, first_1 + 1,
arr_2, first_2 + 1,
length - 1,
initialValue,
op_sum, op_prod))
: initialValue;
}
}
// </editor-fold>
/// A multi-dimensional array
/// Inspired by [orca_array](https://github.com/astrobiology/orca_array)
template
<
typename ValueType, ///< elements' type
std::size_t Dimensions ///< number of dimensions
>
class array
{
// Types ///////////////////////////////////////////////////////////////////////////////////////
public:
// <editor-fold defaultstate="collapsed" desc="STL-like types">
// from <array> (except for `index_type`)
using value_type = ValueType;
using pointer = value_type*;
using const_pointer = const value_type*;
using reference = value_type&;
using const_reference = const value_type&;
using iterator = value_type*;
using const_iterator = const value_type*;
using size_type = std::size_t;
using difference_type = std::ptrdiff_t;
using reverse_iterator = std::reverse_iterator<iterator>;
using const_reverse_iterator = std::reverse_iterator<const_iterator>;
// others
using array_type = array<value_type, Dimensions>;
using const_array_type = const array_type;
using index_type = std::size_t;
// </editor-fold>
// Attributes //////////////////////////////////////////////////////////////////////////////////
// <editor-fold desc="Static Attributes">
public:
/// number of dimensions
static constexpr size_type dimensions = Dimensions;
// </editor-fold>
// <editor-fold desc="Class Attributes">
private:
// ::std::array's are used here mainly (only?) because they are initializable
// from `std::initializer_list` and they support move semantics
// cf. hyper_array::array's constructors
// also ::std::array seem to introduce no overhead over the data they hold
// i.e. sizeof(::std::array<Type, Length>) == sizeof(Type) * Length
/// number of elements in each dimension
::std::array<size_type, Dimensions> _lengths;
/// coefficients to use when computing the index
/// \f[
/// C_i = \begin{cases}
/// \prod_{j=i+1}^{n-2} L_j & i \in [0, n-2] \\
/// 1 & i = n-1
/// \end{cases}
/// \\
/// where \begin{cases}
/// n &: Dimensions - 1 \\
/// C_i &: \_coeffs[i] \\
/// L_j &: \_lengths[j] \\
/// \end{cases}
/// \f]
///
/// @see at()
::std::array<size_type, Dimensions> _coeffs;
/// handles the lifecycle of the dynamically allocated data array
/// The user doesn't need to access it directly
/// If the user needs access to the allocated array, they can use [data()](@ref data())
std::unique_ptr<value_type[]> _dataOwner;
// </editor-fold>
// methods /////////////////////////////////////////////////////////////////////////////////////
public:
// <editor-fold defaultstate="collapsed" desc="Constructors">
/// would it make sense to create an array without specifying the dimension lengths ?
array() = delete;
/// copy-constructor
array(const_array_type& other)
: array(std::move(other.clone()))
{}
/// move constructor
/// allows inclusion of hyper arrays in e.g. STL containers
array(array_type&& other)
: _lengths (std::move(other._lengths))
, _coeffs (std::move(other._coeffs))
, _dataOwner {std::move(other._dataOwner)}
{}
/// the usual way of constructing hyper arrays
template
<
typename... DimensionLengths,
typename = internal::enable_if_t<
sizeof...(DimensionLengths) == Dimensions
&& internal::are_integral<DimensionLengths...>::value>
>
array(DimensionLengths... dimensionLengths)
: _lengths {{static_cast<size_type>(dimensionLengths)...}}
, _coeffs (computeIndexCoeffs(_lengths))
, _dataOwner {allocateData(size())}
{}
/// Creates a new hyper array from "raw data"
array(::std::array<size_type, Dimensions> lengths, ///< length of each dimension
value_type* rawData = nullptr ///< raw data
///< must contain `computeIndexCoeffs(lengths)`
///< if `nullptr`, a new data array will be allocated
)
: _lengths (std::move(lengths))
, _coeffs (computeIndexCoeffs(lengths))
, _dataOwner {rawData == nullptr ? allocateData(size()).release() : rawData}
{}
// </editor-fold>
// <editor-fold defaultstate="collapsed" desc="Assignment Operators">
/// copy assignment
array_type& operator=(const_array_type& other)
{
(*this) = std::move(other.clone());
return *this;
}
/// move assignment
array_type& operator=(array_type&& other)
{
_lengths = std::move(other._lengths);
_coeffs = std::move(other._coeffs);
_dataOwner = std::move(other._dataOwner);
return *this;
}
// </editor-fold>
// <editor-fold defaultstate="collapsed" desc="Whole-Array Iterators">
// from <array>
iterator begin() noexcept { return iterator(data()); }
const_iterator begin() const noexcept { return const_iterator(data()); }
iterator end() noexcept { return iterator(data() + size()); }
const_iterator end() const noexcept { return const_iterator(data() + size()); }
reverse_iterator rbegin() noexcept { return reverse_iterator(end()); }
const_reverse_iterator rbegin() const noexcept { return const_reverse_iterator(end()); }
reverse_iterator rend() noexcept { return reverse_iterator(begin()); }
const_reverse_iterator rend() const noexcept { return const_reverse_iterator(begin()); }
const_iterator cbegin() const noexcept { return const_iterator(data()); }
const_iterator cend() const noexcept { return const_iterator(data() + size()); }
const_reverse_iterator crbegin() const noexcept { return const_reverse_iterator(end()); }
const_reverse_iterator crend() const noexcept { return const_reverse_iterator(begin()); }
// </editor-fold>
/// Creates a deep copy of the hyper array
array_type clone() const
{
return {_lengths, cloneData().release()};
}
/// Returns the length of a given dimension at run-time
size_type length(const size_type dimensionIndex) const
{
if (dimensionIndex >= Dimensions)
{
throw std::out_of_range("The dimension index must be within [0, Dimensions-1]");
}
return _lengths[dimensionIndex];
}
/// Returns a reference to the [_lengths](@ref _lengths) array
const decltype(_lengths)& lengths() const
{
return _lengths;
}
/// Returns the given dimension's coefficient (used for computing the "linear" index)
size_type coeff(const size_type coeffIndex) const
{
if (coeffIndex >= Dimensions)
{
throw std::out_of_range("The coefficient index must be within [0, Dimensions-1]");
}
return _coeffs[coeffIndex];
}
/// Returns a reference to the [_coeffs](@ref _coeffs) array
const decltype(_coeffs)& coeffs() const
{
return _coeffs;
}
/// Returns the total number of elements in [data](@ref data)
constexpr
size_type size() const
{
return internal::ct_accumulate(_lengths,
0,
Dimensions,
static_cast<size_type>(1),
internal::ct_prod<size_type>);
}
/// Returns A constant pointer to the allocated data array
value_type* data()
{
return _dataOwner.get();
}
/// `const` version of [data()](@ref data())
constexpr
const_pointer data() const
{
return _dataOwner.get();
}
/// Returns the element at index `idx` in the data array
reference operator[](const index_type idx)
{
return _dataOwner[idx];
}
/// `const` version of [operator[]](@ref operator[])
constexpr
const_reference operator[](const index_type idx) const
{
return _dataOwner[idx];
}
/// Returns the element at the given index tuple
/// Usage:
/// @code
/// hyper_array::array<double, 3> arr(4, 5, 6);
/// arr.at(3, 1, 4) = 3.14;
/// @endcode
template <typename... Indices>
internal::enable_if_t<sizeof...(Indices) == Dimensions
&& internal::are_integral<Indices...>::value,
reference>
at(Indices... indices)
{
return _dataOwner[rawIndex(indices...)];
}
/// `const` version of [at()](@ref at())
template <typename... Indices>
constexpr
internal::enable_if_t<sizeof...(Indices) == Dimensions
&& internal::are_integral<Indices...>::value,
const_reference>
at(Indices... indices) const
{
return _dataOwner[rawIndex(indices...)];
}
/// Unchecked version of [at()](@ref at())
/// Usage:
/// @code
/// hyper_array::array<double, 3> arr(4, 5, 6);
/// arr(3, 1, 4) = 3.14;
/// @endcode
template <typename... Indices>
internal::enable_if_t<sizeof...(Indices) == Dimensions
&& internal::are_integral<Indices...>::value,
reference>
operator()(Indices... indices)
{
return _dataOwner[rawIndex_noChecks({static_cast<index_type>(indices)...})];
}
/// `const` version of [operator()](@ref operator())
template <typename... Indices>
constexpr
internal::enable_if_t<sizeof...(Indices) == Dimensions
&& internal::are_integral<Indices...>::value,
const_reference>
operator()(Indices... indices) const
{
return _dataOwner[rawIndex_noChecks({static_cast<index_type>(indices)...})];
}
/// Returns the actual index of the element in the data array
/// Usage:
/// @code
/// hyper_array::array<int, 3> arr(4, 5, 6);
/// assert(&arr.at(3, 1, 4) == &arr.data()[arr.rawIndex(3, 1, 4)]);
/// @endcode
template <typename... Indices>
constexpr
internal::enable_if_t<sizeof...(Indices) == Dimensions
&& internal::are_integral<Indices...>::value,
index_type>
rawIndex(Indices... indices) const
{
return rawIndex_noChecks(validateIndexRanges(indices...));
}
private:
template <typename... Indices>
internal::enable_if_t<sizeof...(Indices) == Dimensions
&& internal::are_integral<Indices...>::value,
::std::array<index_type, Dimensions>>
validateIndexRanges(Indices... indices) const
{
::std::array<index_type, Dimensions> indexArray = {{static_cast<index_type>(indices)...}};
// check all indices and prepare an exhaustive report (in oss)
// if some of them are out of bounds
std::ostringstream oss;
for (index_type i = 0; i < Dimensions; ++i)
{
if ((indexArray[i] >= _lengths[i]) || (indexArray[i] < 0))
{
oss << "Index #" << i << " [== " << indexArray[i] << "]"
<< " is out of the [0, " << (_lengths[i]-1) << "] range. ";
}
}
// if nothing has been written to oss then all indices are valid
if (oss.str().empty())
{
return indexArray;
}
else
{
throw std::out_of_range(oss.str());
}
}
constexpr
index_type rawIndex_noChecks(::std::array<index_type, Dimensions>&& indexArray) const
{
// I_{actual} = \sum_{i=0}^{N-1} {C_i \cdot I_i}
//
// where I_{actual} : actual index of the data in the data array
// N : Dimensions
// C_i : _coeffs[i]
// I_i : indexArray[i]
return internal::ct_inner_product(_coeffs, 0,
indexArray, 0,
Dimensions,
static_cast<index_type>(0),
internal::ct_plus<index_type>,
internal::ct_prod<index_type>);
}
static
::std::array<size_type, Dimensions>
computeIndexCoeffs(const ::std::array<size_type, Dimensions>& dimensionLengths)
{
::std::array<size_type, Dimensions> coeffs;
coeffs[Dimensions - 1] = 1;
for (size_type i = 0; i < (Dimensions - 1); ++i)
{
coeffs[i] = internal::ct_accumulate(dimensionLengths,
i + 1,
Dimensions - i - 1,
static_cast<size_type>(1),
internal::ct_prod<size_type>);
}
return coeffs; // hopefully, NRVO should kick in here
}
static
std::unique_ptr<value_type[]> allocateData(const size_type dataSize)
{
#if (__cplusplus < 201402L) // C++14 ?
return std::unique_ptr<value_type[]>{new value_type[dataSize]};
#else
// std::make_unique() is not part of C++11
return std::make_unique<value_type[]>(dataSize);
#endif
}
std::unique_ptr<value_type[]> cloneData() const
{
// allocate the new data container
std::unique_ptr<value_type[]> dataOwner{allocateData(size())};
// copy data to the the new container
std::copy(_dataOwner.get(),
_dataOwner.get() + size(),
dataOwner.get());
//
return dataOwner;
}
};
// <editor-fold desc="orca_array-like declarations">
template<typename ValueType> using array1d = array<ValueType, 1>;
template<typename ValueType> using array2d = array<ValueType, 2>;
template<typename ValueType> using array3d = array<ValueType, 3>;
template<typename ValueType> using array4d = array<ValueType, 4>;
template<typename ValueType> using array5d = array<ValueType, 5>;
template<typename ValueType> using array6d = array<ValueType, 6>;
template<typename ValueType> using array7d = array<ValueType, 7>;
template<typename ValueType> using array8d = array<ValueType, 8>;
template<typename ValueType> using array9d = array<ValueType, 9>;
// </editor-fold>
}
#if HYPER_ARRAY_CONFIG_Overload_Stream_Operator
/// Pretty printing to the standard library's streams
/// Should print something like
/// @code
/// [Dimensions:1];[_lengths: 5 ];[size:5];[_coeffs: 1 ];[data: 0 1 2 3 4 ]
/// @endcode
template <typename T, size_t D>
std::ostream& operator<<(std::ostream& out, const hyper_array::array<T, D>& ha)
{
out << "[Dimensions:" << ha.dimensions << "]";
out << ";[_lengths: ";
for (size_t i = 0; i < ha.dimensions; ++i)
{
out << ha.length(i) << " ";
}
out << "]";
out << ";[size:" << ha.size() << "]";
out << ";[_coeffs: ";
for (size_t i = 0; i < ha.dimensions; ++i)
{
out << ha.coeff(i) << " ";
}
out << "]";
out << ";[data: ";
for (typename hyper_array::array<T, D>::index_type i = 0; i < ha.size(); ++i)
{
out << ha[i] << " ";
}
out << "]";
return out;
}
#endif
main.cpp
// clang++-3.7 -stdlib=libc++ -std=c++11 -Wall -Wextra -Wpedantic -Weverything -Wno-c++98-compat -Werror ${file} -lc++ -lc++abi -o -o ${file_path}/${file_base_name}
// g++ -std=c++11 -std=c++11 -fdiagnostics-show-option -Wall -Wextra -Wpedantic -Werror ${file} -o ${file_path}/${file_base_name}
// std
#include <algorithm>
#include <iostream>
#include <iomanip>
#include <numeric>
#include <vector>
// hyper_array
#include "hyper_array/hyper_array.hpp"
using namespace std;
// shorthand for prints a hyper_array
#define printarr(arr) std::cout << #arr << ": " << arr << std::endl;
int main()
{
// size
{
cout << "\nsize\n";
using el_type = double;
constexpr size_t elementCount = 10;
constexpr size_t dataSize = elementCount*sizeof(el_type);
constexpr size_t std_array_overhead = sizeof(std::array<el_type, elementCount>) - dataSize;
constexpr size_t hyper_array_overhead = sizeof(hyper_array::array1d<el_type>);
constexpr size_t std_vector_overhead = sizeof(std::vector<el_type>(elementCount));
cout << "std::array overhead: " << std_array_overhead << " bytes" << endl;
cout << "hyper_array overhead: " << hyper_array_overhead << " bytes" << endl;
cout << "std::vector overhead: " << std_vector_overhead << " bytes" << endl;
}
// 3d array
{
cout << "\n3d array\n";
hyper_array::array3d<double> aa{2, 3, 4};
int c = 0;
for (auto&& x : aa)
{
x = - c++;
}
printarr(aa)
}
// construction, moving, assignment
{
cout << "\nconstruction, moving, assignment\n";
constexpr size_t elementCount = 3;
using ha_type = hyper_array::array1d<double>;
ha_type aa{hyper_array::array1d<double>{elementCount}};
ha_type bb{aa.length(0)};
ha_type cc(2);
for(typename ha_type::index_type i = 0; i < elementCount; ++i)
{
aa[i] = static_cast<double>(elementCount * i);
}
printarr(aa)
bb = std::move(aa);
cc = bb.clone();
bb[0] = -3;
printarr(bb)
printarr(cc)
const ha_type dd(cc);
printarr(dd)
}
// algorithms
{
cout << "\nalgorithms\n";
constexpr size_t dimensions = 3;
using el_type = double;
using ha_type = hyper_array::array<el_type, dimensions>;
const ::std::array<size_t, dimensions> lengths{{2,3,4}};
ha_type aa{lengths};
printarr(aa) // uninitialized
std::iota(aa.begin(), aa.end(), 1);
printarr(aa)
ha_type bb{aa.lengths()};
std::copy(aa.begin(), aa.end(), bb.rbegin());
printarr(bb)
ha_type cc{aa.lengths()};
std::transform(aa.begin(), aa.end(),
bb.begin(),
cc.begin(),
[](el_type a, el_type b) {
return a + b;
});
printarr(cc);
}
// in containers
{
cout << "\nin containers\n";
using el_type = double;
constexpr size_t dims = 2;
using ha_type = hyper_array::array<el_type, dims>;
vector<ha_type> vv;
vv.emplace_back(hyper_array::array<double, dims>{1, 2});
vv.push_back(hyper_array::array2d<double>{3, 4});
vv.push_back(ha_type{5, 6});
vv.push_back({7, 8});
vv.emplace_back(9, 10);
for (auto&& ha : vv)
{
std::iota(ha.begin(), ha.end(), 1);
cout << "vv[" << std::distance(&vv[0], &ha) << "] ";
printarr(ha)
}
}
cout << "\ndone" << endl;
}
New versions of hyper_array
can now be found on Github.
1 Answer 1
Calling your data structure an array is misleading. It is not an array in C++ jargon, because it uses dynamic memory allocation. But it doesn't seem to be a
multidimensional_vector
either since it is lacking inpush_back
. Maybemultidimensional_dyn_array
would fit best.Comments like
//<editor-fold>
are really only useful if you have an editor that understands them. Mine doesn't, so it just makes it harder to read. Since you said that people are supposed to read the header you may want to get rid of them or at least state which editor they are supposed to use.Would it make sense to create an array without specifying the dimension lengths?
It probably does. Changing the dimension or specifying the dimension later makes the data structure more powerful, which is better. Usually you have to pay for power with performance (
std::vector
is more powerful thanstd::array
, but also slower due to dynamic memory allocation), but in your case you don't need to pay extra for this feature so you should provide it.You generally do not need functions such as
clone
because they are built into the language through copy constructor and assignment operator, which should be preferred.array::clone
should be removed.cloneData
is ok since it is private and required, becauseunique_ptr
's copy constructor doesn't do what you need it to do.array_type& operator=(const_array_type& other) { (*this) = std::move(other.clone()); return *this; }
This doesn't make much sense to me. You are given a
const_array_type& other
and need to assign it to*this
. To do this you copyother
and then move the copy? Firstother.clone()
already gives you a temporary, so thestd::move
is redundant. Second you make an unnecessary copy with dynamic memory allocation. What you should be doing is something like this:array_type& operator= (const array_type &other) { _coeffs = other._coeffs; _lengths = other._lengths; _dataOwner = other.cloneData(); return *this; }
You went a bit overboard with the types.
using value_type = ValueType; using pointer = value_type*; using const_pointer = const value_type*; using reference = value_type&; using const_reference = const value_type&; using iterator = value_type*; using const_iterator = const value_type*; using size_type = std::size_t; using difference_type = std::ptrdiff_t; // others using array_type = array<value_type, Dimensions>; using const_array_type = const array_type; using index_type = std::size_t;
I can see the reason to have
iterator
andconst_iterator
since they may be replaced with classes later, but the advantage ofconst_array_type
overconst array_type
escapes me. I looked up what aconst_array_type
is and felt like I wasted my time. I would remove them. People know what aValueType &
is just by looking at it whilereference
conveys a lot less information.Your
cloneData()
function unnecessarily requiresValueType
to be default constructible. Your array really should work withValueType
s such as thisFoo
:struct Foo{ Foo(int){} Foo(Foo &&){} };
You would use placement new to fix that. Not sure if you can make the constructor not require default constructible objects, so this may be a non-issue. Still, invoking the copy constructor instead of default constructor + assignment operator would be more efficient.
length
andcoeff
have range checks in them and throw exceptions when out of range. Sometimes you cannot or don't want to use exceptions and sometimes you know your index is in range and wouldn't want to waste performance. A way to address that is to replace throwing exceptions withassert
s. You get range checks, no exceptions and no performance loss at the same time./// Returns A constant pointer to the allocated data array value_type* data() { return _dataOwner.get(); }
There is nothing constant here, neither the pointer nor the pointee. Probably just a copy/paste error in the comment.
size
seems to be overly complicated.ct_plus
andct_prod
look like useless code to me, but I'm not sure they are.
-
\$\begingroup\$ Thanks for the review. In retrospect, calling the move assignment from the copy assignment was indeed lazy :) Concerning the types, I was trying to imitate
std::array
's naming convention, but I think you're right: everything under// others
would probably just add more confusion to the reader. \$\endgroup\$maddouri– maddouri2015年10月25日 13:29:49 +00:00Commented Oct 25, 2015 at 13:29 -
\$\begingroup\$ Default construction is definitely an issue here. I ruled out placement new because I expect this class to be used like a
std::array
(e.g.std::array< std::array< type, len1 > , len0 >
) which, AFAIK, does require a default constructor. However, I'll see into providing afill()
method and/or constructor that accepts an initialization value (likestd::vector(size_type n, const value_type& val, ...)
) \$\endgroup\$maddouri– maddouri2015年10月25日 13:29:58 +00:00Commented Oct 25, 2015 at 13:29 -
\$\begingroup\$ I agree with your comment regarding exception throwing. It's just that
assert
is disabled in release mode. I'll see how I could implement something like aruntime_assert
. Finally, I've put the size computation in a separate method thinking that it would be a compile-time evaluation... but it's obviously not the case and should just use an attribute computed during construction. (ps: yes,value_type* data()
's comment is definitely a copy/paste error :) ) \$\endgroup\$maddouri– maddouri2015年10月25日 13:30:06 +00:00Commented Oct 25, 2015 at 13:30 -
\$\begingroup\$ @865719
assert
being disabled in release mode is exactly the point. In debug mode you get the range check for debugging, in release mode you take off the training wheels and get full performance. Thats the way it should be. Adding aruntime_assert
just makes people hate you for wasting their CPU-cycles. \$\endgroup\$nwp– nwp2015年10月25日 14:31:50 +00:00Commented Oct 25, 2015 at 14:31
Explore related questions
See similar questions with these tags.
orca_array
didn't strike me as a good example of code design. It looked more like something designed be somebody who uses more C than C++ .___. \$\endgroup\$hyper_array
:) (of course, my own implementation, too, if far from perfect.. which is why I'm posting it here before settling on a version that I could perhaps publish on github or somwhere else...) \$\endgroup\$std::view
. It's only a multidimensional view (it doesn't own the data) and the size is fixed at compile-time but it has some interesting design ideas :p \$\endgroup\$