pop_range Addendum: Revisions from Reviewer Feedback
This document supplements, rather than replaces, the original
pop_range design document. It covers three points a reviewer raised
after that document was published, the verification done for each, the
resulting code changes, and the measured before/after behavior. The
original document's design rationale, history section, and prior-art
research are unchanged and still apply; only the three areas below have
moved since that write-up.
0. Context
The original pop_view<A>::operator* returned a copy (value_type), the
supported container shape was fixed to pop() + top()/front() (i.e.
only stack, queue, priority_queue), and size()/sized_range was
deliberately left unimplemented. A reviewer pointed out that all three of
these were more conservative than necessary. Each point below was checked
independently — against the actual wording of the relevant concepts, and
against a compiled, sanitizer-clean test — before being adopted.
1. operator* doesn't need to copy
The claim. pop_iterator only models input_iterator. Nothing in
that concept requires a previous *it result to remain valid once the
iterator has been incremented — that guarantee belongs to
forward_iterator, not input_iterator. So operator* can return an
rvalue reference straight into the container instead of a copy, the same
technique std::move_iterator uses for the same reason.
Why the claim holds up. The concept that actually constrains
operator*'s return type is indirectly_readable, via
iter_reference_t. It requires common_reference_with relationships
between iter_reference_t, iter_value_t&, and
iter_rvalue_reference_t — it does not require iter_reference_t to be
a prvalue, and std::move_iterator is a standing precedent for returning
an rvalue reference and still satisfying every iterator concept it claims
to model. There's no requirement anywhere in input_iterator's
definition that a dereferenced value keep being valid across ++.
The change:
// before
value_type operator*() const {
return peek(*adaptor_);
}
// after
decltype(auto) operator*() const {
return std::move(peek(*adaptor_));
}
What this changes for callers. auto v = *it; (including a
range-based for's auto v) now selects v's move constructor instead of
its copy constructor, whenever the underlying peek function
(top()/front()/back()) returns a mutable reference. const auto& v = *it; or auto&& v = *it; read the element in place with no copy or
move at all, same as before.
The one case that doesn't change: priority_queue. Its top() is
const-qualified — it has to be, since mutating the top element in place
could break the heap invariant that guarantees the top actually is the
largest (or smallest, per the comparator) element. std::move() applied
to a const T& still only binds to a copy constructor (a move
constructor takes a non-const rvalue reference), so exactly one copy
per element remains structurally unavoidable for priority_queue
specifically — not a shortcoming of pop_range.
Measured proof. An instrumented probe type (counting copies and
moves separately in its constructors) confirmed the following, with the
counters reset immediately before draining (so numbers exclude the pushes
that built the container):
| Container | Copies | Moves | Notes |
|---|---|---|---|
stack<probe> (5 elements) | 0 | 5 | top() is mutable; every extraction moves |
priority_queue<probe> (4 elements) | 4 | 18 | 4 copies = exactly one per element, forced by top() being const. The 18 moves come from priority_queue::pop()'s own internal heap-reordering (std::pop_heap's sift-down) — that work happens regardless of pop_range and isn't something we introduced or could remove. |
2. Generalizing beyond the three container adaptors
The claim. The same pattern works for anything exposing pop_front()
and/or pop_back(), not just the pop()+top()/front() shape that
stack, queue, and priority_queue share — so std::vector,
std::deque, and std::list could be drained the same way.
The change. The concept was widened from one recognized shape to
three, with a strict, shared priority order so that a type satisfying
more than one shape (e.g. deque and list, which have both ends)
can't have the "peek" half and the "consume" half of the code disagree
about which end is in play:
template <typename A>
concept poppable_adaptor =
requires(A& a) {
typename A::value_type;
{ a.empty() } -> std::convertible_to<bool>;
} &&
((has_nullary_pop<A> && (has_top<A> || has_front<A>)) ||
(has_pop_front<A> && has_front<A>) ||
(has_pop_back<A> && has_back<A>));
template <poppable_adaptor A>
decltype(auto) peek(A& a) {
if constexpr (has_nullary_pop<A> && has_top<A>) return a.top();
else if constexpr (has_nullary_pop<A> && has_front<A>) return a.front();
else if constexpr (has_pop_front<A> && has_front<A>) return a.front();
else return a.back();
}
template <poppable_adaptor A>
void consume(A& a) {
if constexpr (has_nullary_pop<A>) a.pop();
else if constexpr (has_pop_front<A> && has_front<A>) a.pop_front();
else a.pop_back();
}
The priority order (nullary pop() first, then pop_front(), then
pop_back()) means: the three original adaptors are unaffected;
vector — which only ever has pop_back() — drains from the back;
deque/list — which have both — drain FIFO from the front by default,
since pop_front() is checked before pop_back().
Measured proof.
vector (back()/pop_back(), LIFO order): 5 4 3 2 1
deque (front()/pop_front(), FIFO order): 1 2 3 4 5
list (front()/pop_front(), FIFO order): 1 2 3 4 5
All three drained to empty() afterward, as expected.
3. sized_range was more available than the original design assumed
The claim. sized_range's requirement that ranges::size(t) be
well-defined independent of evaluating ranges::begin(t) is scoped, in
the standard's own wording, to types that also model forward_range.
pop_view deliberately doesn't model forward_range. So that stability
requirement never actually constrains pop_view, and there's no real
obstacle to giving it a size().
Why the original design got this wrong. The first draft avoided
size() out of a blanket worry that a shrinking, mutation-sensitive
count would violate sized_range's contract in some general sense. On
rereading the concept's actual definition, that concern is specifically
tied to forward_range — for a plain, single-pass input_range, there's
no such invariance clause to violate. "How many elements are left to pop
right now" is exactly the semantics sized_range expects from a
non-forward range.
The change:
[[nodiscard]] auto size() const
requires requires(const A& a) { a.size(); }
{
using size_type = decltype(std::declval<const A&>().size());
return ptr_ ? static_cast<size_type>(ptr_->size()) : size_type{0};
}
Constrained with a requires clause on the member itself, so pop_view<A>
only gains size() — and only models sized_range — for an A that
actually has a .size() member (true for all of stack, queue,
priority_queue, vector, deque, list).
Measured proof. For a stack<int> with 4 elements: view.size() == 4 before any iteration. After manually advancing the iterator twice
(popping two elements): view.size() == 2. Extended the concept
static_asserts to confirm sized_range<pop_view<std::stack<int>>> and
sized_range<pop_view<std::vector<int>>> both hold, alongside the
pre-existing input_iterator/input_range/view/!forward_range
checks, which are unaffected.
4. Net result
All three changes were verified together — not just individually — by
compiling and running the full test suite (g++ 13.3.0, -std=c++20 and
-std=c++23, -Wall -Wextra -fsanitize=address,undefined) after all
three were applied: zero warnings, zero sanitizer reports, all assertions
passing, including the new copy/move counters, the three new container
types, and the two new sized_range static_asserts.
5. Where to find the updated code
The revised pop_range.hpp and its expanded main.cpp test suite
(reflecting all three changes above) are the versions delivered alongside
this addendum. The original design document remains the reference for
the library's overall history, motivation, and rationale sections, which
are unaffected by this round of changes.
