Copilot commented on code in PR #3547:
URL: https://github.com/apache/brpc/pull/3547#discussion_r4021946956
##########
src/bvar/reducer.h:
##########
@@ -449,28 +476,25 @@ class ConcurrentMaxer : public
babylon::GenericsConcurrentMaxer<T> {
ConcurrentMaxer(T default_value) : _default_value(default_value) {}
T value() const {
- T result;
- if (!Base::value(result)) {
- return _default_value;
- }
+ // Base::value() leaves `result' untouched if nothing was counted.
+ T result = _default_value;
+ Base::value(result);
return std::max(result, _default_value);
}
private:
- T _default_value{0};
+ T _default_value{std::numeric_limits<T>::min()};
};
} // namespace detail
// Numerical types supported by babylon counter.
template <typename T>
-class Maxer<T,
std::enable_if<std::is_constructible<detail::ConcurrentMaxer<T>>::value>>
+class Maxer<T, detail::EnableIfBabylonCounter<T>>
Review Comment:
This constraint also selects the Babylon implementation for `Maxer<double>`,
but Babylon 1.4.4 initializes its max comparator with
`std::numeric_limits<T>::min()`. For floating-point `T`, that is the smallest
positive value, so a sample such as `-1.0` is ignored and the reducer returns
the positive sentinel instead of the maximum. Restrict the Babylon Maxer path
to integral types or change the underlying/generic max identity to
`numeric_limits<T>::lowest()`.
##########
src/bvar/reducer.h:
##########
@@ -93,17 +121,18 @@ class BabylonVariable: public Variable {
}
T get_value() const {
+ CHECK(!(butil::is_same<InvOp, VoidOp>::value) || nullptr == _sampler)
+ << "You should not call Reducer<" << butil::class_name_str<T>()
+ << ", " << butil::class_name_str<Op>() << ">::get_value() when a"
+ << " Window<> is used because the operator does not have inverse.";
return _counter.value();
}
T reset() {
- if (BAIDU_UNLIKELY((!butil::is_same<VoidOp, InvOp>::value))) {
- CHECK(false) << "You should not call Reducer<" <<
butil::class_name_str<T>()
- << ", " << butil::class_name_str<Op>() <<
">::get_value() when a"
- << " Window<> is used because the operator does not
have inverse.";
- return get_value();
- }
-
+ // Unlike AgentCombiner::reset_all_agents(), reading and clearing the
babylon
+ // counter are two separate steps, so values added in between are
lost. This
+ // only affects explicit reset() by users: sampling of an operator
without
+ // inverse is the only internal user and it runs in a single thread.
Review Comment:
Because `value()` and `reset()` are separate operations, an `operator<<`
concurrent with this path can be included in the returned value and then
erased, or added after the read and lost by `reset()`. `Reducer::reset()`
otherwise delegates to `AgentCombiner::reset_all_agents()`, which exchanges
values under its lock. Please provide an atomic snapshot-and-clear path (or
synchronize updates with reset) rather than accepting lost updates.
##########
src/butil/containers/optional.h:
##########
@@ -199,14 +199,15 @@ class optional {
_storage.Init(std::move(value));
}
- template <typename... Args,
- std::enable_if<std::is_constructible<T, Args&&...>::value>* =
nullptr>
- explicit optional(const in_place_t, Args&&... args) : _engaged(true) {
+ template <typename... Args, typename std::enable_if<
+ std::is_constructible<T, Args&&...>::value, bool>::type = false>
+ explicit optional(in_place_t, Args&&... args) : _engaged(true) {
Review Comment:
These constraints change compile-time overload selection, but the existing
optional tests do not exercise either in-place constructor. Add a negative
constructibility assertion for `optional<int>` with `in_place_t` and a string
argument, plus a positive initializer-list in-place construction, so
regressions in the corrected `enable_if` and the newly viable initializer-list
overload are caught.
##########
src/butil/memory/scope_guard.h:
##########
@@ -24,7 +24,7 @@
namespace butil {
template<typename Callback,
- typename = std::enable_if<is_result_void<Callback>::value>>
+ typename = typename
std::enable_if<is_result_void<Callback>::value>::type>
Review Comment:
The existing scope-guard tests only instantiate void-returning callbacks, so
they would also pass with the old unconstrained `std::enable_if<...>` class
argument. Add a compile-time detection test for a non-void callback to lock in
that `MakeScopeGuard` is rejected.
--
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]
