Copilot commented on code in PR #13616:
URL: https://github.com/apache/trafficserver/pull/13616#discussion_r3970722923
##########
include/tsutil/Metrics.h:
##########
@@ -94,13 +94,20 @@ class Metrics
static constexpr int METRIC_TYPE_MASK = 0x1FFF;
private:
- using NameAndId = std::tuple<std::string, IdType>;
- using LookupTable = std::unordered_map<std::string_view, IdType>;
- using NameStorage = std::array<NameAndId, MAX_SIZE>;
- using AtomicStorage = std::array<AtomicType, MAX_SIZE>;
- using NamesAndAtomics = std::tuple<NameStorage, AtomicStorage>;
+ using NameAndId = std::tuple<std::string, IdType>;
+ using LookupTable = std::unordered_map<std::string_view, IdType>;
+ using NameStorage = std::array<NameAndId, MAX_SIZE>;
+ using AtomicStorage = std::array<AtomicType, MAX_SIZE>;
+ /// Per slot flag bits, see @c UNLISTED. A parallel array rather than a
member of @c NameAndId
+ /// because an atomic member would make that tuple neither copyable nor
movable, and the slot is
+ /// written there with a tuple assignment.
+ using FlagStorage = std::array<std::atomic<uint8_t>, MAX_SIZE>;
Review Comment:
`FlagStorage` uses `std::atomic<uint8_t>` elements, but `std::atomic`’s
default constructor does not guarantee zero-initialization. If the blob
allocation path relies on value-initialization to start flags at 0, the
UNLISTED bit may start indeterminate and randomly hide metrics. Fix by
explicitly initializing the flag array to 0 when allocating/adding a blob
(e.g., iterating the `FlagStorage` and doing `store(0,
std::memory_order_relaxed)`), rather than relying on default/value
initialization.
##########
include/tsutil/Metrics.h:
##########
@@ -231,43 +299,75 @@ 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.
+ */
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:
This `operator==` can report equality between an exhausted iterator (by
smaller snapshot bound) and another iterator that is *not* exhausted by its own
bound, which breaks equivalence/transitivity expectations for iterators (e.g.,
`a == b` and `a == end` but `b != end`). That can lead to surprising behavior
if these iterators are used with generic iterator-based algorithms beyond the
narrowly intended termination use-case. Consider a design that preserves an
equivalence relation: e.g., make subrange termination use a dedicated sentinel
type that carries the relevant bound (C++20 sentinel pattern), or ensure
iterators are only comparable when they share the same snapshot (and provide an
explicit “snapshot range” object so `begin/end/find` share a common bound).
--
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]
