This is an automated email from the ASF dual-hosted git repository.
wasphin pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/brpc.git
The following commit(s) were added to refs/heads/master by this push:
new 46c21805 Fix ineffective enable_if constraints in bvar and butil
(#3547)
46c21805 is described below
commit 46c218058000d86aaff6eaf475629d6bcb37fcfe
Author: Bright Chen <[email protected]>
AuthorDate: Fri Sep 18 21:46:48 2026 +0800
Fix ineffective enable_if constraints in bvar and butil (#3547)
* Fix ineffective enable_if constraints in bvar and butil
* Add reset UT
* Use std::enable_if_t instead of std::enable_if::type
* Opt BabylonVariable::reset() comment
---
src/butil/containers/optional.h | 49 ++++++++++----------
src/butil/memory/scope_guard.h | 4 +-
src/bvar/reducer.h | 99 ++++++++++++++++++++++++++++++-----------
test/bvar_reducer_unittest.cpp | 66 +++++++++++++++++++++++++++
4 files changed, 165 insertions(+), 53 deletions(-)
diff --git a/src/butil/containers/optional.h b/src/butil/containers/optional.h
index bde0ad2f..e1969584 100644
--- a/src/butil/containers/optional.h
+++ b/src/butil/containers/optional.h
@@ -22,7 +22,7 @@
#include "butil/memory/manual_constructor.h"
// The `optional` for managing an optional contained value,
-// i.e. a value that may or may not be present, is a C++11
+// i.e. a value that may or may not be present, is a C++14
// compatible version of the C++17 `std::optional` abstraction.
// After C++17, `optional` is an alias for `std::optional`.
@@ -145,33 +145,33 @@ public:
optional(optional&& rhs) noexcept = default;
- template <typename U, typename std::enable_if<
+ template <typename U, std::enable_if_t<
!std::is_same<T, U>::value &&
std::is_constructible<T, const U&>::value &&
!internal::is_constructible_convertible_from_optional<T, U>::value &&
- std::is_convertible<const U&, T>::value, bool>::type = false>
+ std::is_convertible<const U&, T>::value, bool> = false>
optional(const optional<U>& rhs) : _engaged(rhs.has_value()) {
if (_engaged) {
_storage.Init(*rhs);
}
}
- template <typename U, typename std::enable_if<
+ template <typename U, std::enable_if_t<
!std::is_same<T, U>::value &&
std::is_constructible<T, const U&>::value &&
!internal::is_constructible_convertible_from_optional<T, U>::value &&
- !std::is_convertible<const U&, T>::value, bool>::type = false>
+ !std::is_convertible<const U&, T>::value, bool> = false>
explicit optional(const optional<U>& rhs) : _engaged(rhs.has_value()) {
if (_engaged) {
_storage.Init(*rhs);
}
}
- template <typename U, typename std::enable_if<
+ template <typename U, std::enable_if_t<
!std::is_same<T, U>::value &&
std::is_constructible<T, U&&>::value &&
!internal::is_constructible_convertible_from_optional<T, U>::value &&
- std::is_convertible<U&&, T>::value, bool>::type = false>
+ std::is_convertible<U&&, T>::value, bool> = false>
optional(optional<U>&& rhs) : _engaged(rhs.has_value()) {
if (_engaged) {
_storage.Init(std::move(*rhs));
@@ -179,11 +179,11 @@ public:
}
}
- template <typename U, typename std::enable_if<
+ template <typename U, std::enable_if_t<
!std::is_same<T, U>::value &&
std::is_constructible<T, U&&>::value &&
!internal::is_constructible_convertible_from_optional<T, U>::value &&
- !std::is_convertible<U&&, T>::value, bool>::type = false>
+ !std::is_convertible<U&&, T>::value, bool> = false>
explicit optional(optional<U>&& rhs) : _engaged(rhs.has_value()) {
if (_engaged) {
_storage.Init(std::move(*rhs));
@@ -199,33 +199,34 @@ public:
_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, std::enable_if_t<
+ std::is_constructible<T, Args&&...>::value, bool> = false>
+ explicit optional(in_place_t, Args&&... args) : _engaged(true) {
_storage.Init(std::forward<Args>(args)...);
}
- template <typename U, typename... Args, typename std::enable_if<
- std::is_constructible<T, std::initializer_list<U>&,
Args&&...>::value>::type>
+ template <typename U, typename... Args, std::enable_if_t<
+ std::is_constructible<T, std::initializer_list<U>&, Args&&...>::value,
+ bool> = false>
optional(in_place_t, std::initializer_list<U> il, Args&&... args)
: _engaged(true) {
_storage.Init(il, std::forward<Args>(args)...);
}
- template <typename U = T, typename std::enable_if<
+ template <typename U = T, std::enable_if_t<
!std::is_same<in_place_t, typename std::decay<U>::type>::value &&
!std::is_same<optional<T>, typename std::decay<U>::type>::value &&
std::is_constructible<T, U&&>::value &&
- std::is_convertible<U&&, T>::value, bool>::type = false>
+ std::is_convertible<U&&, T>::value, bool> = false>
optional(U&& v) : _engaged(true) {
_storage.Init(std::forward<U>(v));
}
- template <typename U = T, typename std::enable_if<
+ template <typename U = T, std::enable_if_t<
!std::is_same<in_place_t, typename std::decay<U>::type>::value &&
!std::is_same<optional<T>, typename std::decay<U>::type>::value &&
std::is_constructible<T, U&&>::value &&
- !std::is_convertible<U&&, T>::value, bool>::type = false>
+ !std::is_convertible<U&&, T>::value, bool> = false>
explicit optional(U&& v) : _engaged(true) {
_storage.Init(std::forward<U>(v));
}
@@ -244,11 +245,11 @@ public:
optional& operator=(optional&& rhs) = default;
// Value assignment operators
- template <typename U = T, typename = typename std::enable_if<
+ template <typename U = T, typename = std::enable_if_t<
!std::is_same<optional<T>, typename std::decay<U>::type>::value &&
!std::is_same<optional<T>, typename remove_cvref<U>::type>::value &&
std::is_constructible<T, U>::value && std::is_assignable<T&, U>::value
&&
- (!std::is_scalar<T>::value || !std::is_same<T, typename
std::decay<U>::type>::value)>::type>
+ (!std::is_scalar<T>::value || !std::is_same<T, typename
std::decay<U>::type>::value)>>
optional& operator=(U&& v) {
reset();
_storage.Init(std::forward<U>(v));
@@ -256,11 +257,11 @@ public:
return *this;
}
- template <typename U, typename = typename std::enable_if<
+ template <typename U, typename = std::enable_if_t<
!std::is_same<T, U>::value &&
!internal::is_constructible_convertible_assignable_from_optional<T,
U>::value &&
std::is_constructible<T, const U&>::value &&
- std::is_assignable<T&, const U&>::value>::type>
+ std::is_assignable<T&, const U&>::value>>
optional& operator=(const optional<U>& rhs) {
if (rhs) {
operator=(*rhs);
@@ -270,11 +271,11 @@ public:
return *this;
}
- template <typename U, typename = typename std::enable_if<
+ template <typename U, typename = std::enable_if_t<
!std::is_same<T, U>::value &&
!internal::is_constructible_convertible_assignable_from_optional<T,
U>::value &&
std::is_constructible<T, U>::value &&
- std::is_assignable<T&, U>::value>::type>
+ std::is_assignable<T&, U>::value>>
optional& operator=(optional<U>&& rhs) {
if (rhs) {
operator=(std::move(*rhs));
diff --git a/src/butil/memory/scope_guard.h b/src/butil/memory/scope_guard.h
index 377819b5..27fc2ba6 100644
--- a/src/butil/memory/scope_guard.h
+++ b/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 = std::enable_if_t<is_result_void<Callback>::value>>
class ScopeGuard;
template<typename Callback>
@@ -33,7 +33,7 @@ ScopeGuard<Callback> MakeScopeGuard(Callback&& callback)
noexcept;
// ScopeGuard is a simple implementation to guarantee that
// a function is executed upon leaving the current scope.
template<typename Callback>
-class ScopeGuard<Callback> {
+class ScopeGuard<Callback, void> {
public:
ScopeGuard(ScopeGuard&& other) noexcept
: _callback(std::move(other._callback))
diff --git a/src/bvar/reducer.h b/src/bvar/reducer.h
index 9fdb1960..8b40fe3f 100644
--- a/src/bvar/reducer.h
+++ b/src/bvar/reducer.h
@@ -30,6 +30,7 @@
#include "bvar/detail/series.h"
#include "bvar/window.h"
#if WITH_BABYLON_COUNTER
+#include <algorithm> // std::max std::min
#include "babylon/concurrent/counter.h"
#endif // WITH_BABYLON_COUNTER
@@ -50,6 +51,33 @@ private:
};
#if WITH_BABYLON_COUNTER
+// babylon counters constrain their value type with a static_assert inside the
+// class body, which is a hard error rather than a substitution failure.
Probing
+// them with std::is_constructible<> hence breaks the build for the types they
+// reject, instead of falling back to the generic implementation. Mirror the
+// constraint here (see babylon/concurrent/counter.h) so that a rejected type
is
+// never used to instantiate a babylon counter.
+// NOTE: the constraint has to be checked in two steps rather than in a single
+// expression, because sizeof() can not be applied to void or to an incomplete
+// type, and `&&' does not help: short-circuiting is about evaluation, an
+// ill-formed sizeof() is still an error.
+template <typename T, bool = std::is_integral<T>::value ||
+ std::is_floating_point<T>::value>
+struct IsBabylonCounterSupported : std::false_type {};
+
+template <typename T>
+struct IsBabylonCounterSupported<T, true>
+ : butil::integral_constant<bool, sizeof(T) <= 8> {};
+
+// `void` if the babylon counter supports T, a substitution failure otherwise.
+// Selects the babylon-backed partial specializations of Adder/Maxer/Miner
below.
+// NOTE: resolving to the *type* is a MUST. std::enable_if<cond> itself is a
type
+// no matter what `cond` is, and Adder<T> means Adder<T, void>, so
specializing on
+// std::enable_if<cond> instead of std::enable_if_t<cond> silently never
matches.
+template <typename T>
+using EnableIfBabylonCounter =
+ std::enable_if_t<IsBabylonCounterSupported<T>::value>;
+
template<typename T, typename Counter, typename Op, typename InvOp>
class BabylonVariable: public Variable {
public:
@@ -58,12 +86,12 @@ public:
BabylonVariable() = default;
- template<typename U = T, typename std::enable_if<
- !std::is_constructible<Counter, U>::value, bool>::type = false>
+ template<typename U = T, std::enable_if_t<
+ !std::is_constructible<Counter, U>::value, bool> = false>
BabylonVariable(U) {}
// For Maxer.
- template<typename U = T, typename std::enable_if<
- std::is_constructible<Counter, U>::value, bool>::type = false>
+ template<typename U = T, std::enable_if_t<
+ std::is_constructible<Counter, U>::value, bool> = false>
BabylonVariable(U default_value) : _counter(default_value) {}
DISALLOW_COPY_AND_MOVE(BabylonVariable);
@@ -93,17 +121,21 @@ public:
}
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
+ // affects both explicit reset() by users and periodic sampling by the
sampler
+ // thread (e.g. Window/series sampling), where concurrent additions
around each
+ // reset may be dropped. This is an accepted trade-off for the babylon
backend:
+ // in statistics/monitoring scenarios such minor loss does not change
the overall
+ // trend, so slightly inaccurate samples are acceptable.
T result = _counter.value();
_counter.reset();
return result;
@@ -370,15 +402,13 @@ public:
#if WITH_BABYLON_COUNTER
// Numerical types supported by babylon counter.
template <typename T>
-class Adder<T,
std::enable_if<std::is_constructible<babylon::GenericsConcurrentAdder<T>>::value>>
+class Adder<T, detail::EnableIfBabylonCounter<T>>
: public detail::BabylonVariable<T, babylon::GenericsConcurrentAdder<T>,
detail::AddTo<T>, detail::MinusFrom<T>> {
public:
typedef T value_type;
-private:
typedef detail::BabylonVariable<T, babylon::GenericsConcurrentAdder<T>,
detail::AddTo<value_type>,
detail::MinusFrom<value_type>> Base;
-public:
typedef detail::AddTo<value_type> Op;
typedef detail::MinusFrom<value_type> InvOp;
typedef typename Base::sampler_type sampler_type;
@@ -449,28 +479,25 @@ public:
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>>
: public detail::BabylonVariable<T, detail::ConcurrentMaxer<T>,
detail::MaxTo<T>, detail::VoidOp> {
public:
typedef T value_type;
-private:
typedef detail::BabylonVariable<T, detail::ConcurrentMaxer<T>,
detail::MaxTo<value_type>, detail::VoidOp>
Base;
-public:
typedef detail::MaxTo<value_type> Op;
typedef detail::VoidOp InvOp;
typedef typename Base::sampler_type sampler_type;
@@ -527,17 +554,35 @@ public:
};
#if WITH_BABYLON_COUNTER
+namespace detail {
+// The min counterpart of ConcurrentMaxer, see there for why the default value
is
+// needed.
+template <typename T>
+class ConcurrentMiner : public babylon::GenericsConcurrentMiner<T> {
+ typedef babylon::GenericsConcurrentMiner<T> Base;
+public:
+ ConcurrentMiner() = default;
+ explicit ConcurrentMiner(T default_value) : _default_value(default_value)
{}
+
+ T value() const {
+ T result = _default_value;
+ Base::value(result);
+ return std::min(result, _default_value);
+ }
+private:
+ T _default_value{std::numeric_limits<T>::max()};
+};
+} // namespace detail
+
// Numerical types supported by babylon counter.
template <typename T>
-class Miner<T,
std::enable_if<std::is_constructible<babylon::GenericsConcurrentMiner<T>>::value>>
- : public detail::BabylonVariable<T, babylon::GenericsConcurrentMiner<T>,
+class Miner<T, detail::EnableIfBabylonCounter<T>>
+ : public detail::BabylonVariable<T, detail::ConcurrentMiner<T>,
detail::MinTo<T>, detail::VoidOp> {
public:
typedef T value_type;
-private:
- typedef detail::BabylonVariable<value_type,
babylon::GenericsConcurrentMiner<T>,
+ typedef detail::BabylonVariable<value_type, detail::ConcurrentMiner<T>,
detail::MinTo<value_type>, detail::VoidOp>
Base;
-public:
typedef detail::MinTo<value_type> Op;
typedef detail::VoidOp InvOp;
typedef typename Base::sampler_type sampler_type;
diff --git a/test/bvar_reducer_unittest.cpp b/test/bvar_reducer_unittest.cpp
index 02923d92..f905e110 100644
--- a/test/bvar_reducer_unittest.cpp
+++ b/test/bvar_reducer_unittest.cpp
@@ -279,6 +279,72 @@ TEST_F(ReducerTest, non_primitive) {
ASSERT_EQ(9, adder.get_value().x);
}
+// The generic Adder/Maxer/Miner keep their data in a shared AgentCombiner and
+// hence expose share_combiner(), while the babylon-backed ones keep a
value-type
+// counter and do not. This tells the two implementations apart.
+template <typename R>
+struct IsBabylonBacked
+ : std::integral_constant<bool, !bvar::detail::HasShareCombiner<R>::value>
{};
+
+// Which implementation backs a given value type is decided by SFINAE at
compile
+// time, so a malformed condition silently makes the babylon-backed partial
+// specializations unreachable without failing any runtime assertion. Assert
the
+// selection statically.
+TEST_F(ReducerTest, babylon_counter_backend) {
+#if WITH_BABYLON_COUNTER
+ static_assert(IsBabylonBacked<bvar::Adder<int> >::value,
+ "Adder<int> should be backed by a babylon counter");
+ static_assert(IsBabylonBacked<bvar::Adder<int64_t> >::value,
+ "Adder<int64_t> should be backed by a babylon counter");
+ static_assert(IsBabylonBacked<bvar::Adder<double> >::value,
+ "Adder<double> should be backed by a babylon counter");
+ static_assert(IsBabylonBacked<bvar::Maxer<int64_t> >::value,
+ "Maxer<int64_t> should be backed by a babylon counter");
+ static_assert(IsBabylonBacked<bvar::Miner<int64_t> >::value,
+ "Miner<int64_t> should be backed by a babylon counter");
+ // babylon counters only support arithmetic types not larger than 8 bytes.
+ // The types they reject must fall back to the generic implementation
rather
+ // than break the build.
+ static_assert(IsBabylonBacked<bvar::Adder<long double> >::value ==
+ (sizeof(long double) <= 8),
+ "Adder<long double> should follow the size constraint");
+ static_assert(!IsBabylonBacked<bvar::Adder<std::string> >::value,
+ "Adder<std::string> should be backed by an AgentCombiner");
+ static_assert(!IsBabylonBacked<bvar::Adder<Foo> >::value,
+ "Adder<Foo> should be backed by an AgentCombiner");
+#else
+ static_assert(!IsBabylonBacked<bvar::Adder<int64_t> >::value,
+ "Adder<int64_t> should be backed by an AgentCombiner");
+ static_assert(!IsBabylonBacked<bvar::Maxer<int64_t> >::value,
+ "Maxer<int64_t> should be backed by an AgentCombiner");
+ static_assert(!IsBabylonBacked<bvar::Miner<int64_t> >::value,
+ "Miner<int64_t> should be backed by an AgentCombiner");
+#endif // WITH_BABYLON_COUNTER
+}
+
+// reset() must return the accumulated value and restore the identity of the
+// operator, which is how Window<> samples an operator without inverse. This
runs
+// against whichever implementation backs the value type, hence it covers the
+// babylon counters as well when WITH_BABYLON_COUNTER is enabled.
+TEST_F(ReducerTest, reset) {
+ bvar::Adder<int> adder;
+ adder << 3 << 1;
+ ASSERT_EQ(4, adder.reset());
+ ASSERT_EQ(0, adder.get_value());
+ adder << 2;
+ ASSERT_EQ(2, adder.reset());
+
+ bvar::Maxer<int> maxer;
+ maxer << 3 << 1;
+ ASSERT_EQ(3, maxer.reset());
+ ASSERT_EQ(std::numeric_limits<int>::min(), maxer.get_value());
+
+ bvar::Miner<int> miner;
+ miner << 3 << 1;
+ ASSERT_EQ(1, miner.reset());
+ ASSERT_EQ(std::numeric_limits<int>::max(), miner.get_value());
+}
+
bool g_stop = false;
struct StringAppenderResult {
int count;
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
