Skip to content

Conversation

@carsonRadtke
Copy link
Member

@carsonRadtke carsonRadtke commented Jan 28, 2026

resolves: #1169

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

Currently the implementation is feature complete and all tests on modern compilers (with supported language versions) are passing.

What needs to be done:

  • Create docs
  • Run dyn_array tests under ASAN (nonblocking: nice to have)
  • Flesh out some more tests (nonblocking: nice to have, almost out of baseball teams)

What is done (this is a running checklist; I'll delete and rewrite this message before merging):

  • 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
  • Tests for custom allocators
  • More constexpr tests using a constexpr allocator
  • Tests for const iterators
  • Sanity-check static_assertions to make sure gsl::dyn_array::iterator is a proper random-access iterator.
  • Sanity-check static_assertions to make sure gsl::dyn_array is (mostly) a proper allocator-aware container
  • Tests for construction of gsl::dyn_array from std::initializer_list
  • Tests/Support for operations on const gsl::dyn_array
  • Support for MSVC's unchecked iterators
  • Tests/Support for reverse iterators
  • Deduction guides for gsl::dyn_array
  • 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 80
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.
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

Copilot reviewed 9 out of 9 changed files in this pull request and generated 4 comments.

Comments suppressed due to low confidence (2)

include/gsl/dyn_array:237

  • The constructors that allocate storage and then call std::fill / std::copy write into the raw memory returned by Allocator::allocate without first constructing the elements, which is undefined behavior for non-trivial T. These constructors should either use uninitialized algorithms together with std::allocator_traits<Allocator> (e.g., uninitialized_fill_n / uninitialized_copy) or explicitly construct each element before assigning to it.
    explicit constexpr dyn_array(size_type count, const T& value, const Allocator& alloc = {})
        : base{count, alloc}
    {
        std::fill(begin(), end(), value);
    }

    GSL_TYPE_IS_ITERATOR(InputIt)
    constexpr dyn_array(InputIt first, InputIt last, const Allocator& alloc = {})
        : base{gsl::narrow<size_type>(std::distance(first, last)), alloc}
    {
        std::copy(first, last, begin());

include/gsl/dyn_array:262

  • dyn_array::operator=(const dyn_array&) constructs a temporary dyn_array and then returns a reference to that temporary (return dyn_array{other};), which leaves callers with a dangling reference. In common patterns like chained assignment (a = b = c;), this causes the second assignment to read from a dyn_array whose storage has already been deallocated, leading to a use-after-free and potential memory corruption. Rewrite this copy-assignment operator to assign into *this (e.g., via a proper copy/copy‑and‑swap implementation) and return *this rather than a temporary.
    constexpr auto operator=(const dyn_array& other) -> dyn_array& { return dyn_array{other}; }

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

Copilot reviewed 9 out of 9 changed files in this pull request and generated 2 comments.

Comments suppressed due to low confidence (1)

include/gsl/dyn_array:79

  • dyn_array_base uses Allocator::allocate and Allocator::deallocate directly without ever invoking std::allocator_traits<Allocator>::construct/destroy on the elements, while dyn_array later reads from and writes to impl().data() via std::fill/std::copy as if it contained fully constructed T objects. For non-implicit-lifetime or non-trivially destructible types this is undefined behavior: elements are never properly constructed and their destructors are never run before deallocation, which can lead to memory corruption or resource/resource-leak issues when dyn_array is instantiated with complex element types under attacker-controlled data. To fix this, either restrict dyn_array to trivial/implicit-lifetime element types or explicitly manage object lifetimes using allocator traits to construct and destroy elements before use and before releasing storage.
        constexpr dyn_array_base(const Allocator& alloc) : Allocator{alloc}, _impl{nullptr, 0} {}
        constexpr dyn_array_base(size_type count, const Allocator& alloc)
            : Allocator{alloc}, _impl{count == 0 ? nullptr : Allocator::allocate(count), count}
        {}

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

Comment on lines +159 to +162
## <a name="H-dyn_array" />`<dyn_array>`

# TODO (@carsonradtke)

Copy link

Copilot AI Jan 29, 2026

Choose a reason for hiding this comment

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

The <dyn_array> section in headers.md is currently just a TODO placeholder, while other headers in this document provide concrete API descriptions. Given that README.md now advertises dyn_array as implemented, this section should be updated to document the main public types and functions before merging.

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

Currently the implementation is feature complete and all tests are
passing under (at least) the following presets:
 - clang-14-debug
 - clang-17-debug
 - clang-20-debug
 - clang-23-debug
(clang 18.1.3 on Ubuntu 24.04)
(Apple Clang 17.0.0 on macOS Tahoe 26.3)
 - msvc-14-debug
 - msvc-17-debug
 - msvc-20-debug
 - msvc-23-debug
(Visual Studio 2026 18.2 (toolset v145))

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
- [x] Tests for const iterators
- [x] Sanity-check static_assertions to make sure
  gsl::dyn_array::iterator is a proper random-access iterator.
- [x] Sanity-check static_assertions to make sure gsl::dyn_array is
  (mostly) a proper allocator-aware container
- [x] Tests for construction of gsl::dyn_array from std::initializer_list
- [x] Tests/Support for operations on const gsl::dyn_array
- [x] Support for MSVC's unchecked iterators
- [x] Tests/Support for reverse iterators
- [x] Deduction guides for gsl::dyn_array

What needs to be done:

- [ ] Run dyn_array tests under ASAN
- [ ] Ensure support for clang, gcc, MSVC
- [ ] Create docs
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