Copilot commented on code in PR #3563:
URL: https://github.com/apache/brpc/pull/3563#discussion_r4080679407
##########
src/bvar/reducer.h:
##########
@@ -78,6 +78,15 @@ template <typename T>
using EnableIfBabylonCounter =
std::enable_if_t<IsBabylonCounterSupported<T>::value>;
+// babylon::GenericsConcurrentMaxer initializes its aggregation value with
+// numeric_limits<T>::min(). For floating-point types this is the smallest
Review Comment:
The comment references `numeric_limits<T>::min()` without the `std::`
qualifier, which can be confusing in a C++ header (and could be misread as a
project-local symbol). Consider updating the comment to
`std::numeric_limits<T>::min()` for clarity/precision.
##########
src/bvar/reducer.h:
##########
@@ -78,6 +78,15 @@ template <typename T>
using EnableIfBabylonCounter =
std::enable_if_t<IsBabylonCounterSupported<T>::value>;
+// babylon::GenericsConcurrentMaxer initializes its aggregation value with
+// numeric_limits<T>::min(). For floating-point types this is the smallest
+// positive normalized value, so an aggregation containing only negative values
+// is treated as empty. Keep floating-point Maxer instances on the generic
+// implementation until babylon fixes its aggregation identity.
+template <typename T>
+using EnableIfBabylonMaxerCounter = std::enable_if_t<
Review Comment:
`EnableIfBabylonMaxerCounter` doesn’t clearly convey the newly introduced
restriction (integral-only). Consider renaming to something like
`EnableIfBabylonIntegralMaxerCounter` (or similar) to make the constraint
obvious at call sites/specializations.
##########
test/bvar_reducer_unittest.cpp:
##########
@@ -182,6 +182,11 @@ TEST_F(ReducerTest, max) {
ASSERT_EQ(30, reducer2.get_value());
reducer2 << std::numeric_limits<int>::max();
ASSERT_EQ(std::numeric_limits<int>::max(), reducer2.get_value());
+
+ bvar::Maxer<double> reducer3;
+ ASSERT_EQ(std::numeric_limits<double>::lowest(), reducer3.get_value());
+ reducer3 << -30.5 << -10.5 << -20.5;
+ ASSERT_EQ(-10.5, reducer3.get_value());
Review Comment:
`ASSERT_EQ` on `double` is brittle in general. Prefer `ASSERT_DOUBLE_EQ` (or
`ASSERT_NEAR` with a tolerance) for floating-point comparisons, even if these
particular values are currently exactly representable, to make the test robust
across platforms/compilers and future changes.
--
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]
