Copilot commented on code in PR #13616:
URL: https://github.com/apache/trafficserver/pull/13616#discussion_r4008287399
##########
include/tsutil/Metrics.h:
##########
@@ -191,87 +253,21 @@ class Metrics
return _storage->valid(id);
}
- // Static methods to encapsulate access to the atomic's
- class iterator
- {
- public:
- using iterator_category = std::input_iterator_tag;
- using value_type = std::tuple<std::string_view, MetricType,
int64_t>;
- using difference_type = ptrdiff_t;
- using pointer = value_type *;
- using reference = value_type &;
-
- iterator(const Metrics &m, IdType pos) : _metrics(m), _it(pos) {}
-
- iterator &
- operator++()
- {
- next();
-
- return *this;
- }
-
- iterator
- operator++(int)
- {
- iterator result = *this;
-
- next();
-
- return result;
- }
-
- value_type
- operator*() const
- {
- std::string_view name;
- MetricType type;
- auto metric = _metrics.lookup(_it, &name, &type);
-
- return std::make_tuple(name, type, metric->_value.load());
- }
-
- bool
- operator==(const iterator &o) const
- {
- return _it == o._it && std::addressof(_metrics) ==
std::addressof(o._metrics);
- }
-
- bool
- operator!=(const iterator &o) const
- {
- return _it != o._it || std::addressof(_metrics) !=
std::addressof(o._metrics);
- }
-
- private:
- void next();
-
- const Metrics &_metrics;
- Metrics::IdType _it;
- };
-
- iterator
- begin() const
- {
- return iterator(*this, 0);
- }
-
- iterator
- end() const
- {
- return iterator(*this, _storage->next_free_id());
- }
-
- iterator
- find(const std::string_view name) const
+ /** Visit every listed metric.
+ *
+ * @a func is called as <tt>func(std::string_view name, MetricType type,
int64_t value)</tt> for
+ * each listed metric, in creation order. Unlisted metrics are skipped, @see
unlist.
+ *
+ * The set walked is fixed when the call begins: a metric created while it
runs is not visited.
+ * Enumeration is deliberately the whole store and nothing less. There is no
cursor to hold, so
+ * nothing can outlive the walk or name a slot the walk would not visit, and
@a func may not
+ * create a metric, which would be an attempt to grow the store from inside
a pass over it.
+ */
+ template <typename F>
+ void
+ for_each(F &&func) const
{
- auto id = lookup(name);
-
- if (id == NOT_FOUND) {
- return end();
- } else {
- return iterator(*this, id);
- }
+ _storage->for_each(std::forward<F>(func));
Review Comment:
This new template uses `std::forward`, but the public header does not
include `<utility>` and currently relies on a transitive include such as
`<variant>`. A consumer compiling `Metrics.h` with a standard-library
implementation that does not expose `<utility>` through those headers can fail
to compile; add the direct include.
##########
include/tsutil/Metrics.h:
##########
@@ -191,87 +253,21 @@ class Metrics
return _storage->valid(id);
}
- // Static methods to encapsulate access to the atomic's
- class iterator
- {
- public:
- using iterator_category = std::input_iterator_tag;
- using value_type = std::tuple<std::string_view, MetricType,
int64_t>;
- using difference_type = ptrdiff_t;
- using pointer = value_type *;
- using reference = value_type &;
-
- iterator(const Metrics &m, IdType pos) : _metrics(m), _it(pos) {}
-
- iterator &
- operator++()
- {
- next();
-
- return *this;
- }
-
- iterator
- operator++(int)
- {
- iterator result = *this;
-
- next();
-
- return result;
- }
-
- value_type
- operator*() const
- {
- std::string_view name;
- MetricType type;
- auto metric = _metrics.lookup(_it, &name, &type);
-
- return std::make_tuple(name, type, metric->_value.load());
- }
-
- bool
- operator==(const iterator &o) const
- {
- return _it == o._it && std::addressof(_metrics) ==
std::addressof(o._metrics);
- }
-
- bool
- operator!=(const iterator &o) const
- {
- return _it != o._it || std::addressof(_metrics) !=
std::addressof(o._metrics);
- }
-
- private:
- void next();
-
- const Metrics &_metrics;
- Metrics::IdType _it;
- };
-
- iterator
- begin() const
- {
- return iterator(*this, 0);
- }
-
- iterator
- end() const
- {
- return iterator(*this, _storage->next_free_id());
- }
-
- iterator
- find(const std::string_view name) const
+ /** Visit every listed metric.
+ *
+ * @a func is called as <tt>func(std::string_view name, MetricType type,
int64_t value)</tt> for
+ * each listed metric, in creation order. Unlisted metrics are skipped, @see
unlist.
+ *
+ * The set walked is fixed when the call begins: a metric created while it
runs is not visited.
+ * Enumeration is deliberately the whole store and nothing less. There is no
cursor to hold, so
+ * nothing can outlive the walk or name a slot the walk would not visit, and
@a func may not
+ * create a metric, which would be an attempt to grow the store from inside
a pass over it.
Review Comment:
Because `Metrics.h` is installed as a public header
(`src/tsutil/CMakeLists.txt:18-21`), removing `Metrics::iterator`, `begin()`,
`end()`, and `find()` is a source-level break for downstream plugins even
though there are no in-tree callers. Please document the full removal and the
`for_each` migration in the v11 API changes (or retain a compatibility path);
otherwise existing consumers have no upgrade notice.
--
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]
