2
\$\begingroup\$

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) with operator() or at() or a "linear" index (actual index in the underlying 1D data array) with operator[],
  • typedefs (templated usings, actually) for dimensions 1 to 9 (it's totally arbitrary. orca_array defines a maximum of 7),
  • Pretty Printing to std::ostream with an overloaded operator<<

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.

Run It Online

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.

\$\endgroup\$
3
  • 2
    \$\begingroup\$ To be honest, 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\$
    – Morwenn
    Oct 25, 2015 at 12:35
  • \$\begingroup\$ That is one of the reasons why I started working on 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\$
    – maddouri
    Oct 25, 2015 at 12:43
  • 1
    \$\begingroup\$ If you're interested in an alternative design to handle arrays with several dimensions, you could have a look at the proposed 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\$
    – Morwenn
    Oct 25, 2015 at 12:46

1 Answer 1

3
\$\begingroup\$
  • 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 in push_back. Maybe multidimensional_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 than std::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, because unique_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 copy other and then move the copy? First other.clone() already gives you a temporary, so the std::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 and const_iterator since they may be replaced with classes later, but the advantage of const_array_type over const array_type escapes me. I looked up what a const_array_type is and felt like I wasted my time. I would remove them. People know what a ValueType & is just by looking at it while reference conveys a lot less information.

  • Your cloneData() function unnecessarily requires ValueType to be default constructible. Your array really should work with ValueTypes such as this Foo:

    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 and coeff 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 with asserts. 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 and ct_prod look like useless code to me, but I'm not sure they are.

\$\endgroup\$
4
  • \$\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
    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 a fill() method and/or constructor that accepts an initialization value (like std::vector(size_type n, const value_type& val, ...)) \$\endgroup\$
    – maddouri
    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 a runtime_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
    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 a runtime_assert just makes people hate you for wasting their CPU-cycles. \$\endgroup\$
    – nwp
    Oct 25, 2015 at 14:31

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.

Not the answer you're looking for? Browse other questions tagged or ask your own question.