Copilot commented on code in PR #13616:
URL: https://github.com/apache/trafficserver/pull/13616#discussion_r3972321831
##########
include/tsutil/Metrics.h:
##########
@@ -194,15 +252,25 @@ class Metrics
// Static methods to encapsulate access to the atomic's
class iterator
{
+ friend class Metrics;
+
+ /// Tag for the end sentinel, which has no position and reads no storage.
+ struct end_tag {
+ };
+
+ // Only Metrics hands these out, through begin(), end() and find(). A
caller that could name an
+ // arbitrary position could name an unlisted one, which iteration must
never visit.
+ explicit iterator(const Metrics &m);
+ iterator(const Metrics &m, IdType pos);
+ iterator(const Metrics &m, end_tag);
Review Comment:
These constructors were previously publicly usable (notably the
`iterator(const Metrics&, IdType)` constructor is removed from the public
section), and are now restricted to `Metrics` via `friend` + private
constructors. That is a source-level API breaking change for any code
constructing iterators directly.\n\n**Recommendation (moderate):** If external
callers are expected to use `Metrics::iterator` (even incidentally), consider
preserving a safe public construction path (e.g., keep the constructor public
but ensure it cannot represent an unlisted position by normalizing unlisted
positions to `end()`), or provide an explicit factory/API that replaces the old
usage while keeping builds from breaking.
##########
include/tsutil/Metrics.h:
##########
@@ -231,43 +299,81 @@ class Metrics
return std::make_tuple(name, type, metric->_value.load());
}
+ /** Equality.
+ *
+ * Three way rather than a plain position compare: any exhausted iterator
equals the end
+ * sentinel, and equals any other exhausted iterator, since two of them
may have skipped a
+ * different number of unlisted slots. Two live iterators still compare by
position.
+ *
+ * Two positional iterators may hold different snapshots, so exhaustion
between them is judged
+ * against the earlier bound. Otherwise a walk could pass its own bound
while a stop iterator
+ * made later was still live: they would never compare equal and @c
operator++ could not make
+ * progress. The sentinel keeps its own answer, since its bound is
meaningless.
+ *
+ * @note Only iterators taken from the same snapshot are meaningfully
comparable with each
+ * other. Because exhaustion is a property of an iterator's own bound,
two taken at different
+ * times can compare equal to each other while disagreeing about @c end,
so this is not a
+ * total equivalence relation and these are not iterators to hand to a
generic algorithm.
+ * Use @c end to test for exhaustion.
+ */
bool
operator==(const iterator &o) const
{
- return _it == o._it && std::addressof(_metrics) ==
std::addressof(o._metrics);
- }
+ if (std::addressof(_metrics) != std::addressof(o._metrics)) {
+ return false;
+ }
- bool
- operator!=(const iterator &o) const
- {
- return _it != o._it || std::addressof(_metrics) !=
std::addressof(o._metrics);
+ if (_end || o._end) {
+ return at_end() == o.at_end();
+ }
+
+ auto const bound = _bound < o._bound ? _bound : o._bound;
+ bool const a = _it >= bound, b = o._it >= bound;
+
+ if (a || b) {
+ return a && b;
+ }
+ return _it == o._it;
}
Review Comment:
`Metrics::iterator` declares `std::input_iterator_tag` but `operator==` is
explicitly designed (per the new comment) to *not* behave like a
standard-conforming iterator equality (e.g., two iterators can compare equal
while one is still dereferenceable under its own bound). This breaks iterator
requirements and can cause incorrect behavior/UB if the iterator is ever used
with standard algorithms that assume iterator equality is an equivalence
relation and that `a == b` implies interchangeable iterator
state.\n\n**Recommendation (mandatory):** Either (1) adjust the iterator so it
truly models an input iterator (make `operator==` a real equivalence relation
and ensure equal iterators have consistent dereference/exhaustion semantics),
or (2) redesign to avoid presenting this as a standard iterator (e.g., provide
an explicit snapshot/range view type with well-defined semantics, or otherwise
prevent/avoid use in generic algorithms).
##########
include/tsutil/Metrics.h:
##########
@@ -194,15 +252,25 @@ class Metrics
// Static methods to encapsulate access to the atomic's
class iterator
{
+ friend class Metrics;
+
+ /// Tag for the end sentinel, which has no position and reads no storage.
+ struct end_tag {
+ };
+
+ // Only Metrics hands these out, through begin(), end() and find(). A
caller that could name an
+ // arbitrary position could name an unlisted one, which iteration must
never visit.
+ explicit iterator(const Metrics &m);
+ iterator(const Metrics &m, IdType pos);
+ iterator(const Metrics &m, end_tag);
+
public:
using iterator_category = std::input_iterator_tag;
Review Comment:
`Metrics::iterator` declares `std::input_iterator_tag` but `operator==` is
explicitly designed (per the new comment) to *not* behave like a
standard-conforming iterator equality (e.g., two iterators can compare equal
while one is still dereferenceable under its own bound). This breaks iterator
requirements and can cause incorrect behavior/UB if the iterator is ever used
with standard algorithms that assume iterator equality is an equivalence
relation and that `a == b` implies interchangeable iterator
state.\n\n**Recommendation (mandatory):** Either (1) adjust the iterator so it
truly models an input iterator (make `operator==` a real equivalence relation
and ensure equal iterators have consistent dereference/exhaustion semantics),
or (2) redesign to avoid presenting this as a standard iterator (e.g., provide
an explicit snapshot/range view type with well-defined semantics, or otherwise
prevent/avoid use in generic algorithms).
##########
include/tsutil/Metrics.h:
##########
@@ -194,15 +252,25 @@ class Metrics
// Static methods to encapsulate access to the atomic's
class iterator
{
+ friend class Metrics;
Review Comment:
These constructors were previously publicly usable (notably the
`iterator(const Metrics&, IdType)` constructor is removed from the public
section), and are now restricted to `Metrics` via `friend` + private
constructors. That is a source-level API breaking change for any code
constructing iterators directly.\n\n**Recommendation (moderate):** If external
callers are expected to use `Metrics::iterator` (even incidentally), consider
preserving a safe public construction path (e.g., keep the constructor public
but ensure it cannot represent an unlisted position by normalizing unlisted
positions to `end()`), or provide an explicit factory/API that replaces the old
usage while keeping builds from breaking.
--
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]
