Fix ineffective enable_if constraints in bvar and butil

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,45 +145,45 @@ class optional {
 
     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));
             rhs.reset();
         }
     }
 
-    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 @@ 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, 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,23 +245,23 @@ class optional {
     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));
         _engaged = true;
         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 @@ class optional {
         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));

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))

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 @@ class SeriesSamplerImpl : public Sampler {
 };
 
 #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 @@ class BabylonVariable: public Variable {
 
     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 @@ 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
+        // 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 @@ class Adder : public Reducer<T, detail::AddTo<T>, detail::MinusFrom<T> > {
 #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 @@ 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>>
     : 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 @@ class Miner : public Reducer<T, detail::MinTo<T> > {
 };
 
 #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;