cmcfarlen commented on PR #13616:
URL: https://github.com/apache/trafficserver/pull/13616#issuecomment-5604958078
Five comments, one real bug. Taking them in order of consequence.
**The subrange across snapshots — real, fixed in e776b65641**
This is a genuine defect and reachable through the public API. Two `find()`
calls with a metric created between them is enough:
```cpp
auto start = m.find("a"); // captures bound B1
Metrics::Counter::create("z"); // store grows
auto stop = m.find("z"); // bound B2 > B1, and its position is >= B1
for (auto it = start; it != stop; ++it) { ... } // never terminates
```
`it` passes `B1`, becomes exhausted against its own bound, `stop` is still
live against `B2`, so they never compare equal and `operator++` cannot make
progress. I wrote the test first, with a step cap so it fails rather than
hanging, and watched it fail.
Fixed by judging exhaustion between two positional iterators against the
earlier of the two bounds, so such a subrange ends at the earlier snapshot. The
sentinel keeps its own answer, since its bound is meaningless — folding it into
the minimum would make every other iterator compare exhausted immediately.
My earlier subrange test missed this because it created both endpoints after
all the metrics, so both held the same bound.
**The `count > 0` coupling — fair, fixed in 1f0f4cacdd**
Filed twice, and right both times: that assertion did depend on the shared
store holding at least one listed metric. In practice slot 0 is always listed
so it could not actually fail, but the coupling is real and pointless. The
section now creates a listed metric of its own and asserts it is observed.
One correction to the suggestion: an anchor placed before the unlisted tail
proves the loop ran, not that iteration reached the tail. The "none of the tail
names appear" assertion is what covers the tail. The comment says only the
former.
**Structured bindings and `-Werror` — not an issue**
The concern is that binding `[name, type, value]` and using only `name`
trips an unused-variable warning under `-Werror`. Neither clang nor gcc warns
per element of a structured binding, and I can show it rather than argue it:
the exact translation unit compiles with
```
-Wall -Werror -Wextra -Wsuggest-override -Wthread-safety ...
```
and is clean. The pattern is also already in the tree — two occurrences in
this same file before this PR, three in `RecCore.cc`. Rewriting to
`std::get<0>(entry)` would diverge from that for no benefit.
**`listed()` const-correctness — declining, but happy to be overruled**
Correct that `NamesAndAtomics const *` would be tighter in a `const` method.
I left it because `lookup(IdType, ...)` and `name()` immediately above it do
exactly the same thing, and changing one of three makes the file less
consistent, not more. It is a worthwhile sweep across all of them as its own
change; say the word if you would rather I just do the one here.
Both commits build and pass `test_tsutil` individually.
--
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]
