cmcfarlen commented on PR #13616:
URL: https://github.com/apache/trafficserver/pull/13616#issuecomment-5607440326
Two to answer, since the `FlagStorage` one is a repeat and already addressed
above.
**Iterator equality is not an equivalence relation — correct, documented in
03452ff5fe**
The counterexample is real and I can be precise about it. With bounds `B1 <
B2` and an iterator `a` holding `B1`, `b` holding `B2`, and `b` positioned in
`[B1, B2)`: `a == b` (both past the shared minimum), `a == end()` (past its own
bound), but `b != end()`. Transitivity fails.
The cause is structural rather than a slip in the comparison: exhaustion is
a property of each iterator's own snapshot, so any iterator-versus-end test is
per-iterator, and no comparison rule over a single type can paper over that.
I looked at the sentinel design you suggest. It does fix it properly —
iterator-versus-iterator becomes pure position equality,
iterator-versus-sentinel carries the end test, and the two are different
relations so nothing is required to hold across them. The cost is that
`end()`'s return type changes in an installed header and
`std::distance(m.begin(), m.end())` stops compiling; there are two such lines
in this file already, predating this PR.
We decided against it for now and documented the limitation instead. These
are `input_iterator_tag`, the only uses in tree are range-for over the whole
store, `std::distance(begin, end)`, and `find() != end()`, and none of them
depends on transitivity. Trading a public type change for a property nothing
relies on did not seem the right call inside this PR. The note on `operator==`
now says plainly that only same snapshot comparisons and comparison against
`end()` are meaningful, and that these should not be handed to a generic
algorithm — so the next person meets the constraint at the point of use rather
than deriving it. If you would rather have the sentinel, it is a contained
change and I am happy to do it.
Worth adding: the min-bound rule this replaced a hang, not correct
behaviour. Before it, that same subrange did not terminate at all.
**Many SECTIONs mutating the singleton store — declining**
The remedy does not do what it is meant to here. Splitting into separate
`TEST_CASE`s would only reduce accumulation if the shared body registered
metrics on each re-run, and this one does not:
```cpp
TEST_CASE("Metrics unlisting", "[libtsapi][Metrics]")
{
auto &m = Metrics::instance();
SECTION(...)
```
The body is a reference binding. Every registration happens inside a
`SECTION`, so it happens exactly once whichever way the cases are split, and
the total is identical. Splitting also would not reset anything — it is the
same process and the same singleton either way, and there is deliberately no
reset hook, since the store never frees a slot.
On volume: the largest fill in this test case is 8 metrics, and it adds a
few dozen in total against a store that holds 8M. The pattern of many sections
over `Metrics::instance()` is also what the rest of this file already does.
The one place this was a real problem was an assertion that depended on the
store holding at least one listed metric from elsewhere, which is fixed in
1f0f4cacdd.
--
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]
