Fix ineffective enable_if constraints in bvar and butil

[chenBright]

What problem does this PR solve?

Issue Number: resolve #3546

Problem Summary:

std::enable_if<cond> is a class type whatever cond is, only std::enable_if<cond>::type
goes away when the condition fails. Three places write the former, so their constraints check
nothing.

• bvar/reducer.h: the babylon backed partial specializations of Adder/Maxer/Miner are
  keyed on std::enable_if<cond> while Adder<T> means Adder<T, void>, so they never
  match and the babylon counters are dead code even when built with
  --define with_babylon_counter=true. Adding the missing ::type is not enough: babylon
  constrains its counters with a static_assert in the class body, so probing them with
  std::is_constructible is a hard error instead of a substitution failure, and the specializations
  then expose three latent defects. Adder::reset() hits an inverted CHECK and never resets,
  Maxer/Miner return 0 instead of the identity of their operator for an empty sampling period,
  and get_value() carries no CHECK so Window<Maxer<> > loses its warning.
• butil/memory/scope_guard.h: the default argument of the primary template and the argument
  list of the partial specialization share the same wrong expression, so they always agree and a
  callback returning non void is silently accepted.
• butil/containers/optional.h: the in place constructor is constrained by std::enable_if<cond>*,
  a valid pointer type whatever cond is, so optional<int> is reported constructible from a string
  literal. The initializer_list in place constructor puts its constrained parameter after a parameter
  pack without a default argument, so it can neither be deduced nor specified and is never viable.

What is changed and the side effects?

Changed:

bvar/reducer.h

• Key the babylon backed specializations on typename std::enable_if<cond>::type.
• Replace the std::is_constructible probe with IsBabylonCounterSupported, a two step trait
  mirroring the constraint of babylon counters. It cannot be one expression because sizeof does
  not apply to void or to an incomplete type.

butil/memory/scope_guard.h

• Resolve the constraint to void and specialize on ScopeGuard<Callback, void>.

butil/containers/optional.h

• Constrain both in place constructors with typename std::enable_if<cond, bool>::type = false,
  the form already used elsewhere in the file.

Side effects:

• Performance effects:

• Breaking backward compatibility:

---

Check List:

• Please make sure your changes are compilable.
• When providing us with a new feature, it is best to add related tests.
• Please follow Contributor Covenant Code of Conduct.

[Copilot]

🟡 Changes recommended

Two critical reducer issues remain unresolved, involving concurrent reset updates and floating-point Maxer identity.

Get a fresh assessment by requesting another Copilot review.

Pull request overview

This PR fixes ineffective enable_if constraints in bvar reducers, ScopeGuard, and optional in-place constructors.

Changes:

• Corrects Babylon reducer detection and specialization constraints.
• Fixes ScopeGuard and optional constructor SFINAE.
• Adds compile-time backend-selection coverage.

File summaries

File	Summary
test/bvar_reducer_unittest.cpp	Verifies Babylon backend selection.
src/bvar/reducer.h	Corrects Babylon support detection. Critical (3 votes): reset can lose concurrent updates. Critical (1 vote): Babylon Maxer<double> uses an invalid floating-point identity.
src/butil/memory/scope_guard.h	Corrects callback constraints. Nit (3 votes): add coverage rejecting non-void callbacks.
src/butil/containers/optional.h	Corrects both in-place constructor constraints. Nits (1, 1, and 2 votes): add negative constructibility and positive initializer-list tests.

Review details

Suppressed comments (2)

src/butil/containers/optional.h:204

• The optional tests do not exercise this constructor's constraint. Add a compile-time assertion that optional<int> is not constructible from (in_place, "text"); otherwise the original pointer-to-enable_if bug could regress while the runtime tests still pass.

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

src/butil/containers/optional.h:210

• No test covers the initializer-list in-place overload. Add a successful compile/runtime case such as optional<std::vector<int>>(in_place, {1, 2}) to ensure the defaulted constraint remains viable; this was previously undeducible.

    template <typename U, typename... Args, typename std::enable_if<
        std::is_constructible<T, std::initializer_list<U>&, Args&&...>::value,
        bool>::type = false>

• Files reviewed: 4/4 changed files
• Comments generated: 4
• Review effort level: Lite

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[Copilot]

🟡 Changes recommended

Add regression tests covering invalid constructions, initializer-list emplacement, non-void scope guards, and Babylon reset behavior.

Get a fresh assessment by requesting another Copilot review.

Review details

Suppressed comments (4)

src/butil/containers/optional.h:204

• The corrected in-place SFINAE is not covered by a compile-time regression test: the existing optional tests only exercise valid constructions (for example, optional<Obj>(in_place)), so reverting to the unconstrained pointer form or leaving the initializer-list parameter undeducible would still pass. Add assertions for rejection of an invalid argument (such as optional<int> from a string) and for the now-viable initializer-list in-place overload.

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

src/butil/containers/optional.h:204

• The corrected in-place constraint is not covered by the existing optional tests: they do not assert that optional<int> is not constructible from (in_place, "literal"). Since the previous pointer-shaped enable_if made exactly that trait report true, add a compile-time negative test here.

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

src/butil/containers/optional.h:210

• The initializer-list constructor is newly made viable, but test/optional_unittest.cpp only tests make_optional with a scalar and does not instantiate this overload. Add a construction test such as an optional vector with in_place and an initializer list so the defaulted constraint and forwarding path are compiled.

    template <typename U, typename... Args, typename std::enable_if<
        std::is_constructible<T, std::initializer_list<U>&, Args&&...>::value,
        bool>::type = false>

src/butil/memory/scope_guard.h:28

• The new class-template constraint has no regression test for the invalid case: the existing scope-guard tests only use void-returning callbacks, so the previous implementation would continue to pass. Add a compile-time detection/assertion that a callback returning a non-void value cannot form MakeScopeGuard (and retain the existing valid callback test).

template<typename Callback,
         typename = typename std::enable_if<is_result_void<Callback>::value>::type>
class ScopeGuard;

• Files reviewed: 4/4 changed files
• Comments generated: 1
• Review effort level: Lite

[Copilot]

🔵 Needs a closer look

Moderate test-coverage findings remain for optional in-place overloads and non-void scope guards.

Review details

Suppressed comments (4)

src/butil/containers/optional.h:210

• The regression fixed here is compile-time overload participation, but the existing optional tests do not exercise either in-place overload. Add checks that optional<int> is not constructible from (in_place, "x") and that an initializer-list case such as optional<std::vector<int>>(in_place, {1, 2}) is constructible; otherwise reverting the ::type/default-parameter fixes can pass the suite unnoticed.

    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) {
        _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,
        bool>::type = false>

src/butil/containers/optional.h:204

• The variadic in-place constraint is corrected here, but test/optional_unittest.cpp only covers optional<Obj>(in_place) and does not verify that an invalid argument such as a string literal is rejected. Add a compile-time std::is_constructible regression assertion for this overload.

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

src/butil/containers/optional.h:210

• The initializer-list overload is also newly made viable, but no existing optional test exercises construction such as optional<vector<int>>(in_place, {1, 2}) or its rejection path. Add a regression test covering both successful initializer-list construction and an unsupported element/argument combination.

    template <typename U, typename... Args, typename std::enable_if<
        std::is_constructible<T, std::initializer_list<U>&, Args&&...>::value,
        bool>::type = false>

src/butil/memory/scope_guard.h:27

• The existing scope-guard unit test only exercises valid void callbacks and cannot catch the original ineffective constraint. Add a compile-time detection/negative case proving that MakeScopeGuard is not viable for a callback returning a value; otherwise this SFINAE fix can regress without a test failure.

         typename = typename std::enable_if<is_result_void<Callback>::value>::type>

• Files reviewed: 4/4 changed files
• Comments generated: 0 new
• Review effort level: Lite