From dc12124aba71a99e2519e80b0c4b659662922b73 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 1/2] 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
---
 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(

From 57d2fa4e523e3d565ad551fcdeae01ef7c11cda0 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Maciej=20=C5=BBenczykowski?= <maze@google.com>
Date: Fri, 24 Apr 2020 11:21:21 -0700
Subject: [PATCH 2/2] result.h - fix bugprone-suspicious-semicolon warning
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Fixes:
  system/core/base/include/android-base/result.h:
  133:94: warning: potentially unintended semicolon [bugprone-suspicious-semicolon]

Bernie says:
  it probably means that there's a parser bug with "if constexpr"

  maybe, at static analysis pass, the "if constexpr" was evaluated to false,
  and the compiler removed the "then" block from the AST...
  ... and then it thought you had written it that way :-)
  https://reviews.llvm.org/D46027

Test: builds
Bug: 153035880
Signed-off-by: Maciej Żenczykowski <maze@google.com>
Change-Id: I25df8eeca4ec06b3180c1cd21b554fc583c5581a
---
 base/include/android-base/result.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/base/include/android-base/result.h b/base/include/android-base/result.h
index 5e65876c5..56a4f3e80 100644
--- a/base/include/android-base/result.h
+++ b/base/include/android-base/result.h
@@ -130,6 +130,7 @@ class Error {
 
   template <typename T>
   Error& operator<<(T&& t) {
+    // NOLINTNEXTLINE(bugprone-suspicious-semicolon)
     if constexpr (std::is_same_v<std::remove_cv_t<std::remove_reference_t<T>>, ResultError>) {
       errno_ = t.code();
       return (*this) << t.message();