Skip to main content

pop_range Addendum: Revisions from Reviewer Feedback

· 6 min read
Austin Kim
Staff Software Engineer, System programming and media framework specialist

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):

ContainerCopiesMovesNotes
stack<probe> (5 elements)05top() is mutable; every extraction moves
priority_queue<probe> (4 elements)4184 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.