-
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.
8ab08e0 to
2076fb9
Compare
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
2076fb9 to
8acdd5a
Compare
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:
What needs to be done: