Skip to content

Conversation

@carsonRadtke
Copy link
Member

resolves: #1169

Implement gsl::dyn_array<T, Allocator> as specified by the CppCoreGuidlines here:
https://github.com/isocpp/CppCoreGuidelines/blob/master/docs/dyn_array.md

What is done:

  • Basic Functionality
  • Tests of basic functionality
  • Tests for constexpr functions
  • Tests/Support for ranges
  • Ensure support for C++14, C++17, C++20, and C++23

What needs to be done:

  • Tests for custom allocators
  • More constexpr tests using a constexpr allocator
  • Tests/Support for operations on const gsl::dyn_array
  • Tests/Support for MSVC's unchecked iterators
  • Tests/Support for construction of gsl::dyn_array from std::initializer_list
  • Tests/Support for reverse iterators
  • Deduction guides for gsl::dyn_array
  • Sanity-check static_assertions to make sure gsl::dyn_array is (mostly) a proper allocator-aware container
  • Sanity-check static_assertions to make sure gsl::dyn_array::iterator is a proper random-access iterator.
  • Run dyn_array tests under ASAN
  • Ensure support for clang, gcc, MSVC

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Implements a new gsl::dyn_array<T, Allocator> type, adds supporting feature-detection/utility macros, and introduces a dedicated test suite for the new container.

Changes:

  • Add gsl::dyn_array implementation and iterator type in include/gsl/dyn_array, including allocator-aware storage, range-based constructors, and basic accessors.
  • Extend include/gsl/util with feature macros for ranges/concepts, constexpr support, and a GSL_TYPE_IS_ITERATOR helper plus a pre-C++20 is_iterator trait.
  • Add tests/dyn_array_tests.cpp to exercise basic construction, indexing, iteration, ranges interoperability, algorithms, and limited constexpr behavior.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 10 comments.

File Description
include/gsl/dyn_array Introduces the core dyn_array container and its iterator, but currently has several correctness issues (missing include of gsl/util for required macros, raw allocation without proper element construction/destruction, broken copy-assignment, a mis-declared operator== intended as move assignment, and const accessors that break const-correctness and rely on const_cast).
include/gsl/util Adds macros (GSL_HAS_RANGES, GSL_HAS_CONCEPTS, GSL_CONSTEXPR_ALLOCATOR, GSL_CONSTEXPR_SINCE_CPP20, GSL_TYPE_IS_ITERATOR) and an is_iterator trait; however, the new code depends on <iterator> and concept types without including <iterator> and has a minor mismatch in an #endif comment.
tests/dyn_array_tests.cpp Provides a good initial test suite for basic dyn_array functionality and ranges integration, but includes one ineffective std::all_of call (begin(), begin()) and a misleading #endif comment, and does not yet cover copy assignment or const-usage paths that are currently mis-implemented in the header.
Comments suppressed due to low confidence (3)

include/gsl/dyn_array:151

  • The postfix increment/decrement operators for dyn_array_iterator return dyn_array_iterator& and mutate the same object before returning it, whereas standard iterator semantics (and the existing span_iterator in this codebase) return the previous iterator value by value. This divergence can cause subtle issues with algorithms that rely on the usual post-increment semantics; it would be safer and more consistent to follow the span_iterator pattern and return a copy.
        constexpr auto operator++(int) -> dyn_array_iterator&
        {
            auto& rv = *this;
            ++(*this);
            return rv;
        }

        constexpr auto operator--() -> dyn_array_iterator&
        {
            Expects(_pos > 0);
            _pos--;
            return *this;
        }

        constexpr auto operator--(int) -> dyn_array_iterator&
        {
            auto& rv = *this;
            --(*this);
            return rv;
        }

include/gsl/dyn_array:245

  • The copy-assignment operator currently constructs and returns a new dyn_array (return dyn_array{other};) instead of assigning into *this, and it returns a dyn_array& reference to a temporary. This is ill-formed and does not provide the usual copy-assignment semantics; the operator should either be deleted or implemented to reassign the current object and then return *this.
    constexpr dyn_array(const dyn_array& other) : dyn_array(other.begin(), other.end()) {}

    constexpr auto operator=(const dyn_array& other) -> dyn_array& { return dyn_array{other}; }

include/gsl/dyn_array:80

  • dyn_array_base allocates and deallocates raw storage via Allocator::allocate/Allocator::deallocate but never constructs or destroys the contained T objects, while higher-level code (dyn_array constructors and operator[]) treats that storage as fully-constructed T instances. For any non-trivially constructible or destructible T (e.g., types with non-trivial constructors, destructors, or virtual functions), this results in undefined behavior when std::fill/std::copy write into the uninitialized storage and when clients access elements via operator[], which can corrupt object state or vtables and be exploitable for memory corruption or data exfiltration in security‑sensitive code. To avoid this, ensure that elements are properly constructed and destroyed (e.g., via std::allocator_traits<Allocator>::construct/destroy or by constraining T to trivial types and documenting that restriction) before they are read or written through dyn_array's API.
    template <typename T, typename Allocator = std::allocator<T>>
    class dyn_array_base : private Allocator
    {
        using pointer = typename dyn_array_traits<T>::pointer;
        using size_type = typename dyn_array_traits<T>::size_type;

        const class dyn_array_impl
        {
        public:
            constexpr dyn_array_impl(pointer data, size_type count) : _data{data}, _count{count}
            {
                Ensures((_count == 0 && _data == nullptr) || (_count > 0 && _data != nullptr));
            }

            constexpr auto data() const { return _data; }

            constexpr auto count() const { return _count; }

        private:
            pointer _data;
            size_type _count;
        } _impl;

    public:
        constexpr dyn_array_base() : _impl{nullptr, 0} {}
        constexpr dyn_array_base(size_type count)
            : _impl{count == 0 ? nullptr : Allocator::allocate(count), count}
        {}

        GSL_CONSTEXPR_SINCE_CPP20 ~dyn_array_base()
        {
            if (impl().data()) Allocator::deallocate(impl().data(), impl().count());
        }

        constexpr auto impl() const -> const dyn_array_impl& { return _impl; }

Comment on lines 69 to 79
public:
constexpr dyn_array_base() : _impl{nullptr, 0} {}
constexpr dyn_array_base(size_type count)
: _impl{count == 0 ? nullptr : Allocator::allocate(count), count}
{}

GSL_CONSTEXPR_SINCE_CPP20 ~dyn_array_base()
{
if (impl().data()) Allocator::deallocate(impl().data(), impl().count());
}
Copy link

Copilot AI Jan 28, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

dyn_array_base allocates and deallocates raw storage via Allocator::allocate/deallocate but never constructs or destroys elements; the code then writes to that storage using std::fill/std::copy. For non–implicit-lifetime types this yields undefined behavior because the object lifetime is never started, so this class should either be constrained to implicit-lifetime/trivially constructible T or it should use std::allocator_traits<Allocator>::construct/destroy (and appropriate loops) to manage element lifetimes.

Copilot uses AI. Check for mistakes.
@carsonRadtke carsonRadtke force-pushed the dyn_array branch 3 times, most recently from 8ab08e0 to 2076fb9 Compare January 28, 2026 19:03
Implement gsl::dyn_array<T, Allocator> as specified by the
CppCoreGuidlines here:
https://github.com/isocpp/CppCoreGuidelines/blob/master/docs/dyn_array.md

What is done:

- [x] Basic Functionality
- [x] Tests of basic functionality
- [x] Tests for constexpr functions
- [x] Tests/Support for ranges
- [x] Ensure support for C++14, C++17, C++20, and C++23
- [x] Tests for custom allocators
- [x] More constexpr tests using a constexpr allocator

What needs to be done:

- [ ] Tests/Support for operations on const gsl::dyn_array
- [ ] Tests/Support for MSVC's unchecked iterators
- [ ] Tests/Support for construction of gsl::dyn_array from std::initializer_list
- [ ] Tests/Support for reverse iterators
- [ ] Tests for const iterators
- [ ] Deduction guides for gsl::dyn_array
- [ ] Sanity-check static_assertions to make sure gsl::dyn_array is
  (mostly) a proper allocator-aware container
- [ ] Sanity-check static_assertions to make sure
  gsl::dyn_array::iterator is a proper random-access iterator.
- [ ] Run dyn_array tests under ASAN
- [ ] Ensure support for clang, gcc, MSVC
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Status: In Progress Currently being worked on

Projects

None yet

Development

Successfully merging this pull request may close these issues.

gsl::dyn_array<T, Deleter>?

1 participant