Copilot commented on code in PR #13616:
URL: https://github.com/apache/trafficserver/pull/13616#discussion_r3973504948
##########
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>`, whose default constructor does
not guarantee zero-initialization. If the code doesn’t explicitly `store(0)`
for each slot (both when a blob is created and when a new slot is allocated),
flags can start with indeterminate values and metrics may appear randomly
unlisted. Ensure flags are deterministically initialized (e.g., clear the
entire `FlagStorage` when allocating a blob, and explicitly clear the flag for
each newly allocated slot in `Storage::create`).
##########
include/tsutil/Metrics.h:
##########
@@ -231,43 +299,80 @@ 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 A snapshot is the sequence: iterators from different ones are no
more comparable than
+ * iterators into different containers, and mixing them is unspecified.
Within one snapshot
+ * equality is the equivalence relation an input iterator requires. The
rule above keeps the
+ * unspecified case terminating rather than hanging.
+ */
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 change appears to rely on `operator!=` being available via C++20
rewritten comparisons (since `operator!=` was removed but call sites/tests
still use `!=`). If any build targets still compile this code as C++17 (or
earlier), `a != b` will fail to compile without an explicit `operator!=`.
Either confirm the project is C++20+ everywhere, or re-introduce `operator!=`
as `return !(*this == o);` to preserve compatibility.
--
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]
