cmcfarlen commented on PR #13616: URL: https://github.com/apache/trafficserver/pull/13616#issuecomment-5609330865
Both of these arrived twice; answering once each. **`input_iterator_tag` versus the note — fair, and my note caused it. Reframed in 0d16232576** The objection lands on the previous wording rather than on the code: declaring `std::input_iterator_tag` and then saying "not iterators to hand to a generic algorithm" advertises a contract and denies it in the same breath. That was mine and it was the wrong framing. The framing that fits what the code does: **a snapshot is the sequence.** `begin()` fixes the extent to be walked; iterators from two different snapshots are no more comparable than iterators into two different containers, and the standard already treats that as outside the domain rather than as a broken relation. Within one snapshot every iterator shares a bound, so equality is exactly the equivalence relation an input iterator requires, and the tag is honest. The min-bound rule then has a narrower job than the old note implied: it makes the out-of-domain case *terminate* instead of hang. Before it, that comparison did not terminate at all, which is a worse kind of unspecified. On the two suggested remedies: the sentinel design does fix the relation properly and I priced it out — `end()`'s return type changes in an installed header and `std::distance(m.begin(), m.end())` stops compiling, which is two lines in this file that predate the PR. We decided that was not a trade to make inside this PR. It remains a contained change if a maintainer prefers it. **Private iterator constructors are a source break — correct, and now documented** Accurate: `iterator(const Metrics &, IdType)` was public and is not any more. I am keeping it private. Nothing in tree constructs an iterator directly, and there is no sensible reason to — a caller holding an id wants `lookup`, not an iterator positioned at one slot. More to the point, that public constructor is where both iterator defects found in this review came from: a gauge id whose type bits put it past the bound, and a subrange built from two snapshots that never terminated. Both required naming an arbitrary position. The suggested compromise, keeping it public but normalising unlisted positions to `end()`, closes neither. What was missing was disclosure, so the PR description now has an **API change** section recording it, for the 11.0.0 release notes alongside the `createSpan` and `rename` removals from #13583. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
