Copilot commented on code in PR #13583:
URL: https://github.com/apache/trafficserver/pull/13583#discussion_r3927325038
##########
include/tsutil/Metrics.h:
##########
@@ -318,13 +308,28 @@ class Metrics
return (t << METRIC_TYPE_BITS | blob << 16 | offset);
}
+ /// As @c _makeId, without the type bits.
+ static constexpr uint32_t
+ _pack(uint16_t blob, uint16_t offset)
+ {
+ return static_cast<uint32_t>(blob) << 16 | offset;
+ }
+
+ // A packed position must not reach the type bits, and an offset must fit
its field.
+ static_assert(MAX_SIZE <= 0x10000);
+ static_assert(MAX_BLOBS <= (1 << (METRIC_TYPE_BITS - 16)));
+
class Storage
{
- BlobStorage _blobs;
- uint16_t _cur_blob = 0;
- uint16_t _cur_off = 0;
- LookupTable _lookups;
- mutable std::mutex _mutex;
+ /* The next free slot, packed as @c _makeId packs one. A single value
because a reader that
+ * caught a new offset against an old blob index, or the reverse, would
reject ids that exist
+ * or accept ids that do not. Release stored last, after the blob pointer
or the slot's name it
+ * publishes. Only ever increases, so an id is allocated exactly when it
packs below it.
+ */
Review Comment:
The Storage::_next_free comment says it is “packed as _makeId packs one”,
but _next_free actually stores a position packed via _pack() (no metric-type
bit). This is a concrete mismatch between the comment and the implementation
and makes the publication/ordering contract harder to reason about for future
readers.
--
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]
