chenBright commented on code in PR #3547:
URL: https://github.com/apache/brpc/pull/3547#discussion_r4022020844
##########
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:
`Maxer<double>` already behaves exactly like this without the babylon
backend: the
generic `Maxer` initializes its identity with `std::numeric_limits<T>::min()`
(`reducer.h:442`), so `MaxTo` drops `-1.0` and `get_value()` returns the
positive
sentinel. `detail::ConcurrentMaxer` mirrors that identity on purpose, so both
implementations return the same value for the same input, which is the
property
this PR is about. babylon's comparer behaves consistently with it: it
initializes
`result` to `EXTREMUM = numeric_limits<T>::min()` and reports no result when
every
sample compares below it, at which point the wrapper returns the identity.
Restricting the babylon path to integral types would make the two
implementations
disagree instead of removing the sentinel. Switching to
`numeric_limits<T>::lowest()`
is the right fix, but it changes the long-standing behavior of the generic
`Maxer<double>` as well, so I would rather do it in a separate PR that fixes
both
paths and adds a floating point test.
--
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]
