Copilot commented on code in PR #3129:
URL: https://github.com/apache/brpc/pull/3129#discussion_r2463260568
##########
src/bvar/multi_dimension.h:
##########
@@ -162,11 +175,8 @@ class MultiDimension : public MVariable<KeyType> {
template <typename K>
static typename std::enable_if<!butil::is_same<K, key_type>::value>::type
insert_metrics_map(MetricMap& bg, const K& labels_value, op_value_type
metric) {
- key_type labels_value_str;
- for (auto& label : labels_value) {
- // key_type::value_type must be able to convert to std::string.
- labels_value_str.push_back(std::string(label));
- }
+ // key_type::value_type must be able to convert to std::string.
+ key_type labels_value_str(labels_value.cbegin(), labels_value.cend());
Review Comment:
This range-based constructor assumes KeyType supports construction from
iterators, which may not be true for all container types (e.g., std::array).
The previous loop-based approach was safer for heterogeneous types. Consider
retaining the loop or adding a static_assert to verify iterator-based
construction is supported.
```suggestion
key_type labels_value_str;
for (auto it = labels_value.cbegin(); it != labels_value.cend();
++it) {
labels_value_str.push_back(*it);
}
```
##########
src/bvar/multi_dimension_inl.h:
##########
@@ -137,58 +138,60 @@ void MultiDimension<T,
KeyType>::list_stats(std::vector<key_type>* names) {
}
}
-template <typename T, typename KeyType>
+template <typename T, typename KeyType, bool Shared>
template <typename K>
-T* MultiDimension<T, KeyType>::get_stats_impl(const K& labels_value) {
+typename MultiDimension<T, KeyType, Shared>::value_ptr_type
+MultiDimension<T, KeyType, Shared>::get_stats_impl(const K& labels_value) {
if (!is_valid_lables_value(labels_value)) {
- return nullptr;
+ return NULL;
}
MetricMapScopedPtr metric_map_ptr;
if (_metric_map.Read(&metric_map_ptr) != 0) {
LOG(ERROR) << "Fail to read dbd";
- return nullptr;
+ return NULL;
}
auto it = metric_map_ptr->seek(labels_value);
- if (it == nullptr) {
- return nullptr;
+ if (NULL == it) {
+ return NULL;
}
return (*it);
}
-template <typename T, typename KeyType>
+template <typename T, typename KeyType, bool Shared>
template <typename K>
-T* MultiDimension<T, KeyType>::get_stats_impl(
+typename MultiDimension<T, KeyType, Shared>::value_ptr_type
+MultiDimension<T, KeyType, Shared>::get_stats_impl(
const K& labels_value, STATS_OP stats_op, bool* do_write) {
if (!is_valid_lables_value(labels_value)) {
- return nullptr;
+ return NULL;
}
{
MetricMapScopedPtr metric_map_ptr;
if (0 != _metric_map.Read(&metric_map_ptr)) {
LOG(ERROR) << "Fail to read dbd";
- return nullptr;
+ return NULL;
}
auto it = metric_map_ptr->seek(labels_value);
if (NULL != it) {
return (*it);
} else if (READ_ONLY == stats_op) {
- return nullptr;
+ return NULL;
}
if (metric_map_ptr->size() > _max_stats_count) {
LOG(ERROR) << "Too many stats seen, overflow detected, max stats
count="
<< _max_stats_count;
- return nullptr;
+ return NULL;
}
}
- // Because DBD has two copies(foreground and background) MetricMap, both
copies need to be modify,
+ // Because DBD has two copies(foreground and background) MetricMap, both
copies need to be modified,
// In order to avoid new duplicate bvar object, need use cache_metric to
cache the new bvar object,
// In this way, when modifying the second copy, can directly use the
cache_metric bvar object.
op_value_type cache_metric = NULL;
- auto insert_fn = [&labels_value, &cache_metric, &do_write](MetricMap& bg) {
+ auto insert_fn = [this, &labels_value, &cache_metric,
&do_write](MetricMap& bg) {
Review Comment:
Capturing `this` in the lambda is unnecessary and potentially dangerous. The
lambda already has access to the member function context. Remove `this` from
the capture list.
```suggestion
auto insert_fn = [&labels_value, &cache_metric, &do_write](MetricMap&
bg) {
```
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
