From 62ae965b0a79c258ad2e6b773c885fbed8de9679 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Maciej=20=C5=BBenczykowski?= <maze@google.com>
Date: Fri, 24 Apr 2020 11:15:03 -0700
Subject: [PATCH] expected.h - fix bugprone-forwarding-reference-overload
 warnings
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Fixes:
  system/core/base/include/android-base/expected.h:
  186:13: warning: constructor accepting a forwarding reference can hide the copy and move constructors [bugprone-forwarding-reference-overload]
  195:22: warning: constructor accepting a forwarding reference can hide the copy and move constructors [bugprone-forwarding-reference-overload]
  611:13: warning: constructor accepting a forwarding reference can hide the copy and move constructors [bugprone-forwarding-reference-overload]

To quote Tom Cherry:
  I'm a bit confused at what's happening there.
  I think it's a bug in the linter itself.
  The general solution to that problem is a heavy dose of std::enable_if<>
  to hide that constructor when the 'U' parameter is the same class,
  but those constructors do have the necessarily std::enable_if<> lines.

  I think the problem is that the linter doesn't check that the macro
  _ENABLE_IF() expands into std::enable_if<>.  Let me try explicitly
  putting the std::enable_if<> instead of the macro and check if it
  goes away.

  I expanded the macro but the linter doesn't still doesn't accept
  the format of `std::enable_if_t<(condition_here)>* = nullptr`.
  It does accept `typename Enable = std::enable_if_t<(condition_here), void>`,
  which is the syntax used on their example here:
    https://clang.llvm.org/extra/clang-tidy/checks/bugprone-forwarding-reference-overload.html.

  That latter syntax doesn't work for us.
  See the Notes section on
    https://en.cppreference.com/w/cpp/types/enable_if
  as a reference for why what we're doing is correct.

Test: builds
Bug: 153035880
Signed-off-by: Maciej Żenczykowski <maze@google.com>
Change-Id: I493ff19208cc104f5f176a36ec23fbcb914388f7
Merged-In: I493ff19208cc104f5f176a36ec23fbcb914388f7
---
 base/include/android-base/expected.h | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/base/include/android-base/expected.h b/base/include/android-base/expected.h
index 38f0d6f2b..9470344e8 100644
--- a/base/include/android-base/expected.h
+++ b/base/include/android-base/expected.h
@@ -182,7 +182,7 @@ class _NODISCARD_ expected {
                 !std::is_same_v<unexpected<E>, std::remove_cv_t<std::remove_reference_t<U>>> &&
                 std::is_convertible_v<U&&, T> /* non-explicit */
                 )>
-  // NOLINTNEXTLINE(google-explicit-constructor)
+  // NOLINTNEXTLINE(google-explicit-constructor,bugprone-forwarding-reference-overload)
   constexpr expected(U&& v) : var_(std::in_place_index<0>, std::forward<U>(v)) {}
 
   template <class U = T _ENABLE_IF(
@@ -192,6 +192,7 @@ class _NODISCARD_ expected {
                 !std::is_same_v<unexpected<E>, std::remove_cv_t<std::remove_reference_t<U>>> &&
                 !std::is_convertible_v<U&&, T> /* explicit */
                 )>
+  // NOLINTNEXTLINE(bugprone-forwarding-reference-overload)
   constexpr explicit expected(U&& v) : var_(std::in_place_index<0>, T(std::forward<U>(v))) {}
 
   template<class G = E _ENABLE_IF(
@@ -607,7 +608,7 @@ class unexpected {
                 std::is_constructible_v<E, Err> &&
                 !std::is_same_v<std::remove_cv_t<std::remove_reference_t<E>>, std::in_place_t> &&
                 !std::is_same_v<std::remove_cv_t<std::remove_reference_t<E>>, unexpected>)>
-  // NOLINTNEXTLINE(google-explicit-constructor)
+  // NOLINTNEXTLINE(google-explicit-constructor,bugprone-forwarding-reference-overload)
   constexpr unexpected(Err&& e) : val_(std::forward<Err>(e)) {}
 
   template<class U, class... Args _ENABLE_IF(