-
Notifications
You must be signed in to change notification settings - Fork 760
feat: implementation of gsl::dyn_array #1228
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this 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_arrayimplementation and iterator type ininclude/gsl/dyn_array, including allocator-aware storage, range-based constructors, and basic accessors. - Extend
include/gsl/utilwith feature macros for ranges/concepts, constexpr support, and aGSL_TYPE_IS_ITERATORhelper plus a pre-C++20is_iteratortrait. - Add
tests/dyn_array_tests.cppto 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_iteratorreturndyn_array_iterator&and mutate the same object before returning it, whereas standard iterator semantics (and the existingspan_iteratorin 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 thespan_iteratorpattern 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 adyn_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_baseallocates and deallocates raw storage viaAllocator::allocate/Allocator::deallocatebut never constructs or destroys the containedTobjects, while higher-level code (dyn_arrayconstructors andoperator[]) treats that storage as fully-constructedTinstances. For any non-trivially constructible or destructibleT(e.g., types with non-trivial constructors, destructors, or virtual functions), this results in undefined behavior whenstd::fill/std::copywrite into the uninitialized storage and when clients access elements viaoperator[], 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., viastd::allocator_traits<Allocator>::construct/destroyor by constrainingTto trivial types and documenting that restriction) before they are read or written throughdyn_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; }
| 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()); | ||
| } |
Copilot
AI
Jan 28, 2026
There was a problem hiding this comment.
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.
bb8b58e to
ee30bbe
Compare
There was a problem hiding this 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::copywrite into the raw memory returned byAllocator::allocatewithout first constructing the elements, which is undefined behavior for non-trivialT. These constructors should either use uninitialized algorithms together withstd::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 temporarydyn_arrayand 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 adyn_arraywhose 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*thisrather than a temporary.
constexpr auto operator=(const dyn_array& other) -> dyn_array& { return dyn_array{other}; }
0c59648 to
cd047be
Compare
There was a problem hiding this 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_baseusesAllocator::allocateandAllocator::deallocatedirectly without ever invokingstd::allocator_traits<Allocator>::construct/destroyon the elements, whiledyn_arraylater reads from and writes toimpl().data()viastd::fill/std::copyas if it contained fully constructedTobjects. 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 whendyn_arrayis instantiated with complex element types under attacker-controlled data. To fix this, either restrictdyn_arrayto 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());
}
| ## <a name="H-dyn_array" />`<dyn_array>` | ||
|
|
||
| # TODO (@carsonradtke) | ||
|
|
Copilot
AI
Jan 29, 2026
There was a problem hiding this comment.
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.
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
cd047be to
e52da3f
Compare
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:
What is done (this is a running checklist; I'll delete and rewrite this message before merging):