Complete the random access iterator interface of dyn_array_iterator

[Jeff Lenamon (lenamonj)]

Fixes #1270.

dyn_array_iterator declares random_access_iterator_tag but had no <, >, <=, >=, -> or n + it, and its default constructor was guarded on __cpp_lib_ranges. std::sort over a dyn_array did not compile. The relational operators check the same contract as operator==, -> checks the same as *, n + it forwards to it + n, and the default constructor is unconditional, as the forward iterator requirements ask in every supported standard.

Two tests cover each new operator, std::sort, and default construction of both iterator types; they fail to compile on main.

[Copilot]

🟢 Approval recommended

The code changes appear correct and well-covered by tests, with only a minor test-file comment needing alignment with the new includes.

Pull request overview

This PR completes the details::dyn_array_iterator random-access iterator surface so that its declared std::random_access_iterator_tag is accurate and standard algorithms (notably std::sort) compile and work correctly with gsl::dyn_array.

Changes:

• Make dyn_array_iterator default-constructible unconditionally (per forward-iterator requirements across supported standards).
• Add missing random-access iterator operators: relational comparisons (<, >, <=, >=), operator->, and n + it.
• Add unit tests covering the new operators, std::sort on dyn_array, and default-construction of iterator types.

File summaries

File	Description
include/gsl/dyn_array	Adds missing random-access iterator operators and makes the iterator default constructor unconditional.
tests/dyn_array_tests.cpp	Adds tests for the new iterator operators, std::sort, and default-construction of iterators.

Review details

Suppressed comments (1)

tests/dyn_array_tests.cpp:17

• The comment about testing transitive inclusion is now inaccurate: this file explicitly includes , but the comment says is not included directly. Please update the comment (or remove it) so it matches the current include list and test intent.

#include <utility>

// Despite using <algorithm> and <ranges> utilities in this test, they
// are not being included directly by this file as a test to ensure
// transitive inclusion via <gsl/dyn_array>.

• Files reviewed: 2/2 changed files
• Comments generated: 0
• Review effort level: Lite

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[Carson Radtke (carsonRadtke)]

The changes LGTM, but I'd like to see a couple of things added:

1. Can we add a static_assert to the test suite to make sure that the iterator satisfies the random_access_iterator concept?
2. Can we add a test to confirm that a std::span can be made from a gsl::dyn_array? This requires std::contiguous_iterator, so we may need to adapt the implementation.

[Jeff Lenamon (lenamonj)]

Both added.

static_asserts for random_access_iterator and contiguous_iterator, on iterator and const_iterator, sit beside the existing input_iterator one, plus ranges::contiguous_range. Three changes to the iterator were needed to make them pass.

operator[] had a non-const overload returning reference and a const one returning const_reference, so subscripting a const iterator gave const T& where the concept requires iter_reference_t. It is now a single const overload taking difference_type, matching operator*() const.

iterator_concept is contiguous_iterator_tag under the same guard span_iterator uses.

std::pointer_traits is specialised the way span_iterator specialises it. Without that hook std::to_address(end()) goes through operator->, whose Expects(_pos < _end_pos) terminates on the end iterator.

The span test checks span.data() == data() and to_address(end()) == data() + size(). The direct <algorithm> include from the first commit is gone again; std::sort reaches the test through <gsl/dyn_array>, as the comment at the top of the file describes.

Built and tested at C++14, 17, 20 and 23 with gcc and clang; the span test is compiled out below 20.

[Copilot]

🟡 Changes recommended

to_address performs undefined arithmetic for default and empty iterators.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

• Files reviewed: 2/2 changed files
• Comments generated: 1
• Review effort level: Balanced