2
\$\begingroup\$

I am working with a code base where unfortunately I cannot replace all raw arrays with std::array. The spec provides no guarantee that the memory layout of a std::array is the same as a raw array meaning it can't be a drop in replacement in all situations (the data() method won't help you out if your code expects a pointer to a raw multidimensional array for example).

I have a lot of templates that are designed to work with Standard Library containers and I don't want to modify them to also accept raw arrays. To this end I have written a non-owning wrapper class for raw arrays which will allow them to be treated as a Standard Library container.

The interface attempts to mimic that of std::array. I would appreciate any feedback on the implementation but specifically areas where it may not meet the expectation of somebody using a Standard Library container.

The code compiles under C++17.

namespace fibb
{
    template <typename T, size_t N>
    class Array_Wrapper
    {
        public:
            /* TYPES */
            using value_type = T;
            using pointer = value_type*;
            using const_pointer = const pointer;
            using reference = value_type&;
            using const_reference = const reference;
            using size_type = size_t;
            using difference_type = ptrdiff_t;
            using iterator = pointer;
            using const_iterator = const_pointer;
            using reverse_iterator = std::reverse_iterator<iterator>;
            using const_reverse_iterator = std::reverse_iterator<const_iterator>;

            /* CONSTRUCTORS */
            Array_Wrapper(T (&array_)[N]) // sized array
                : m_array(array_)
            { }

            Array_Wrapper(T*& array_) // decayed array pointer
                : m_array(array_)
            { }

            /* COMPARISON */
            bool operator==(const Array_Wrapper& other) const { return m_array == other.m_array; }
            bool operator!=(const Array_Wrapper& other) const { return m_array != other.m_array; }

            /* FILL / SWAP */
            void fill(const value_type& val) { std::fill_n(begin(), N, val); }

            void swap(Array_Wrapper& other) noexcept(std::is_nothrow_swappable_v<value_type>)
            {
                if (*this != other)
                {
                    std::swap_ranges(begin(), end(), other.begin());
                }   
            }

            void swap(std::array<value_type, N>& other) noexcept(std::is_nothrow_swappable_v<value_type>)
            {
                // swap ranges expects 3 iterators of the same type
                // iterators for std::array are implementation defined
                // the data() method always returns a raw ptr
                std::swap_ranges(begin(), end(), other.data());
            }

            /* ITERATORS */
            constexpr iterator begin() noexcept { return m_array; }
            constexpr const_iterator begin() const noexcept { return m_array; }
            constexpr const_iterator cbegin() const noexcept { return m_array; }

            constexpr iterator end() noexcept { return m_array + N; }
            constexpr const_iterator end() const noexcept { return m_array + N; }
            constexpr const_iterator cend() const noexcept { return m_array + N; }

            constexpr reverse_iterator rbegin() noexcept { return reverse_iterator(end()); }
            constexpr const_reverse_iterator rbegin() const noexcept { return const_reverse_iterator(cend()); }
            constexpr const_reverse_iterator crbegin() const noexcept { return const_reverse_iterator(cend()); }

            constexpr reverse_iterator rend() noexcept { return reverse_iterator(begin()); }
            constexpr const_reverse_iterator rend() const noexcept { return const_reverse_iterator(cbegin()); }
            constexpr const_reverse_iterator crend() const noexcept { return const_reverse_iterator(cbegin()); }

            /* CAPACITY */
            constexpr size_type size() const noexcept { return N; }
            constexpr size_type max_size() const noexcept { return N; }
            constexpr bool empty() const noexcept { return N; }

            /* ELEMENT ACCESS */
            constexpr reference operator[](size_type pos) { return m_array[pos]; }
            constexpr const_reference operator[](size_type pos) const { return m_array[pos]; }

            constexpr reference at(size_type pos)
            {
                if (pos >= N)
                {
                    throw std::out_of_range(std::string("Out of range: ") + std::to_string(pos));
                }
                return m_array[pos];
            }

            constexpr const_reference at(size_type pos) const
            {
                if (pos >= N)
                {
                    throw std::out_of_range(std::string("Out of range: ") + std::to_string(pos));
                }
                return m_array[pos];
            }

            constexpr reference front() { return m_array[0]; }
            constexpr const_reference front() const { return m_array[0]; }
            constexpr reference back() { return m_array[N - 1]; }
            constexpr const_reference back() const { return m_array[N - 1]; }
            constexpr pointer data() noexcept { return m_array; }
            constexpr const_pointer data() const noexcept { return m_array; }

        private:
            pointer m_array;
    };
}

Example construction:

constexpr size_t SIZE = 5;

int arr[SIZE] = {1, 3, 5, 7, 9};

fibb::Array_Wrapper wrap1(arr);

int* decayed_arr = arr;

fibb::Array_Wrapper<int, SIZE> wrap2(decayed_arr);

assert(&arr[0] == wrap1.begin() && &arr[0] == wrap2.begin());

EDIT:

Suggested corrections can be found on my personal github. The code has been abandoned in favour of gsl::span as recommended.

\$\endgroup\$
  • \$\begingroup\$ Any specific reason you can't just pass the raw array to the algorithms instead? I.e. is the only reason for this wrapper to provide container trait types (like ArrayWrapper::value? Otherwise, I don't see much of a restriction for passing raw C-style arrays directly (like this). \$\endgroup\$ – hoffmale Jun 10 '18 at 23:31
  • \$\begingroup\$ Unless I'm missing something you can't pass a decayed pointer into that? std::begin and std::end require a sized array. \$\endgroup\$ – Fibbles Jun 10 '18 at 23:54
  • \$\begingroup\$ That's why I asked for clarification, your explanations never mention decayed pointers (only the example, which I interpreted as a "nice to have" extra). \$\endgroup\$ – hoffmale Jun 11 '18 at 0:17
  • \$\begingroup\$ Yeh sorry about that. It needs to work with decayed pointers. I'm also sure some of the code base is pre C++11 so expects things like begin() and end() methods rather than using the stand alone functions. In an ideal world it'd all be refactored but that's not possible at present. \$\endgroup\$ – Fibbles Jun 11 '18 at 0:25
  • 1
    \$\begingroup\$ It sounds like you want an array_view. You might find a complete implementation of one knocking about the ISO proposals and among that community. Also, Boost.Range v2 has an array wrapper; I don’t recall how general it is but it is specifically used for C strings that are decayed pointers. \$\endgroup\$ – JDługosz Jun 11 '18 at 3:07
3
\$\begingroup\$
  • Array_Wrapper::const_pointer is the same as T *const (const pointer to T), instead of the intended const T* (pointer to const T). Same for ArrayWrapper::const_reference. This allows the contents of m_array to be modified via const_iterator or const_reference if T isn't const itself!

  • All member functions (including constructors) could be marked constexpr in C++20. In C++17, only fill and both overloads for swap wouldn't work, since std::fill_n and std::swap_range aren't constexpr yet.

  • The implementation changes semantics for different operations. Comparison uses referential equality (= reference semantics), whereas swapping has value semantics (= swaps all values inside the arrays). Please choose one option, and stay consistent!

  • Many member functions have their preconditions violated if N == 0. Maybe specialize for N = 0, or use SFINAE to hide invalid operations?

  • Similar operations often include code duplication (e.g. at or operator==/operator!=). Try to extract common code, or try to implement one operation in terms of another.

  • Nearly all member functions (except the overloads for at) can be marked noexcept (though a lot are already).

  • #includes are missing.

  • Array_Wrapper::empty() returns false if N == 0 and trueotherwise. This doesn't seem intended.

  • I'd expect some documentation for Array_Wrappers shortcomings, i.e.

    • it doesn't take ownership of the array!
    • it doesn't work (nor is it intended to) with dynamically allocated arrays whose size is only known at runtime. (At least people used to gsl::span or the proposed std::span would expect this functionality.)

Final note: Are there any issues with using gsl::span? It would give all the features in this implementation, plus some more (e.g. subspans, wrapping runtime arrays, ...).

\$\endgroup\$
  • 1
    \$\begingroup\$ Also, please run some exhaustive tests on your implementation. Many defects could have been caught by simple test cases. (Remember: "It compiles" is not sufficient if member functions in a class template aren't actually instantiated) \$\endgroup\$ – hoffmale Jun 11 '18 at 4:18
  • \$\begingroup\$ Thanks for the feedback. I omitted the includes for brevity, they are there in the header. Likewise there is some brief documentation on non-ownership but individual method docs are non existent because the intent was to provide something that behaved as if it were a std::array. AFAIK zero sized arrays are illegal (even though std::array has a special case for N == 0). This is why there is no specific check for it. I have egg on my face over the empty() method and will be reviewing my unit tests. As for gsl::span I wasn't aware of it previously but will have a good look at it. \$\endgroup\$ – Fibbles Jun 11 '18 at 16:07
  • \$\begingroup\$ @Fibbles: The problem with "behaves as std::array" is that std::array is directly responsible for memory management of the whole array (RAII). Since the wrapper does not provide this property, it should be explicitly mentioned (with all the same pitfalls like std::string_view). And while there is no explicit provision for zero-length arrays in the standard, most compilers have an extension that supports them. If you don't want to support zero-length arrays in your wrapper, just make an empty definition for N == 0, like this: template<typename T> class Array_Wrapper<T, 0>; \$\endgroup\$ – hoffmale Jun 11 '18 at 16:50
  • \$\begingroup\$ I have taken onboard your comments and tried to make the documentation more explicit in what features of std::array are mimicked and which are not. I had thought describing it as a "non-owning wrapper" was enough to clarify it was not responsible for memory management but you are right that it is better to be explicit. As for the N == 0 case, I have just added a static assert to the constructors as it is more readable. \$\endgroup\$ – Fibbles Jun 11 '18 at 17:08
  • \$\begingroup\$ I have made the amendments you recommended. They are available on my personal github. I won't be continuing with it further though as I have started using gsl::span as you suggested. \$\endgroup\$ – Fibbles Jun 11 '18 at 19:56
1
\$\begingroup\$

I assume since this is being made to order that the semantics are designed for your specific use case. But, document that to be clear! In particular, it is odd that swap copies the elements like an owning container, as opposed to just swapping the references. Audit to make sure all semantics are as-intended and drawn from the real use case. Better to leave something unavailable (then add it once you find it is needed) then to get the use-case wrong.


Array_Wrapper(T (&array_)[N]) // sized array
    : m_array(array_)
{ }

The current syntax is to use uniform initialization. That is, curly braces rather than parentheses for the member init list.

Array_Wrapper(T*& array_) // decayed array pointer
    : m_array(array_)
{ }

I don’t see you doing anything with N here. You must specify it as an explicit template argument? I would have expected something more like gsl::span which includes a run-time length if it could not be determined at compile time.

Actually, is there a reason why gsl::span doesn’t do what you want?


void fill(const value_type& val) { std::fill_n(begin(), N, val); }

Fat interface: a generic range-aware fill should work on your type.


constexpr reference at(size_type pos)
{
    if (pos >= N)
    {
        throw std::out_of_range(std::string("Out of range: ") + std::to_string(pos));
    }
    return m_array[pos];
}

The throw (and the string formatting!) generates a lot of code. The happy path is trivially inlined and optimized over. So keep the cold path out of the middle of it!

In general, put your throw in a helper function, and take precautions to make sure that helper does not get inlined, and the compiler knows it does not return. It can be shared among all template instantiations, too.

Also, (see e.g. Microsoft’s GSL implementation) having the throw in one place makes it handy to replace the error behavior, add your client’s logging system, etc.

constexpr reference at(size_type pos)
{
    static_assert (std::is_unsigned_v<size_type> >) // instead of checking pos<0 too
    if (pos >= N)
        raise_range_error (pos, N);
    return m_array[pos];
}

Just check out the code generated when you use (your original) at! shocking.

\$\endgroup\$
  • \$\begingroup\$ Thanks for the feedback. The swap semantics are as they are because the interface should behave as if it were a std::array. Swapping pointers in the wrapper would not alter the underlying arrays in any way which is not how std::array works. Similarly fill() is there because std::array has it. Specifying length at run time is not something needed at present but you are right that it is probably better to have the flexibility. @hoffmale also mentioned gsl::span. It's not something I was aware of but will definitely look into. Your comment on the exception handling was eye opening. \$\endgroup\$ – Fibbles Jun 11 '18 at 16:20

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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