From 62f84e94c8a362a6b3bb4827cb818dee11d8ecf3 Mon Sep 17 00:00:00 2001
From: Daniel Bertalan <dani@danielbertalan.dev>
Date: Mon, 5 Jul 2021 18:23:45 +0200
Subject: [PATCH] AK+Kernel: Fix perfect forwarding constructors shadowing
 others

If a non-const lvalue reference is passed to these constructors, the
converting constructor will be selected instead of the desired copy/move
constructor.

Since I needed to touch `KResultOr` anyway, I made the forwarding
converting constructor use `forward<U>` instead of `move`. This meant
that previously, if a lvalue was passed to it, a move operation took
place even if no `move()` was called on it. Member initializers and
if-else statements have been changed to match our current coding style.
---
 AK/Function.h    |  4 ++--
 Kernel/KResult.h | 22 ++++++++++++++--------
 2 files changed, 16 insertions(+), 10 deletions(-)

diff --git a/AK/Function.h b/AK/Function.h
index e553bd5e86e..a5bc0489f7e 100644
--- a/AK/Function.h
+++ b/AK/Function.h
@@ -62,13 +62,13 @@ public:
     }
 
     template<typename CallableType>
-    Function(CallableType&& callable) requires((IsFunctionObject<CallableType> && IsCallableWithArguments<CallableType, In...>))
+    Function(CallableType&& callable) requires((IsFunctionObject<CallableType> && IsCallableWithArguments<CallableType, In...> && !IsSame<RemoveCVReference<CallableType>, Function>))
     {
         init_with_callable(forward<CallableType>(callable));
     }
 
     template<typename FunctionType>
-    Function(FunctionType f) requires((IsFunctionPointer<FunctionType> && IsCallableWithArguments<RemovePointer<FunctionType>, In...>))
+    Function(FunctionType f) requires((IsFunctionPointer<FunctionType> && IsCallableWithArguments<RemovePointer<FunctionType>, In...> && !IsSame<RemoveCVReference<FunctionType>, Function>))
     {
         init_with_callable(move(f));
     }
diff --git a/Kernel/KResult.h b/Kernel/KResult.h
index 793d65438ed..05ae90ba25e 100644
--- a/Kernel/KResult.h
+++ b/Kernel/KResult.h
@@ -58,24 +58,30 @@ public:
     }
 
     ALWAYS_INLINE KResultOr(T&& value)
+        : m_have_storage(true)
     {
         new (&m_storage) T(move(value));
-        m_have_storage = true;
+    }
+
+    ALWAYS_INLINE KResultOr(const T& value)
+        : m_have_storage(true)
+    {
+        new (&m_storage) T(value);
     }
 
     template<typename U>
-    ALWAYS_INLINE KResultOr(U&& value)
+    ALWAYS_INLINE KResultOr(U&& value) requires(!IsSame<RemoveCVReference<U>, KResultOr<T>>)
+        : m_have_storage(true)
     {
-        new (&m_storage) T(move(value));
-        m_have_storage = true;
+        new (&m_storage) T(forward<U>(value));
     }
 
     KResultOr(KResultOr&& other)
     {
         m_is_error = other.m_is_error;
-        if (m_is_error)
+        if (m_is_error) {
             m_error = other.m_error;
-        else {
+        } else {
             if (other.m_have_storage) {
                 new (&m_storage) T(move(other.value()));
                 m_have_storage = true;
@@ -96,9 +102,9 @@ public:
             m_have_storage = false;
         }
         m_is_error = other.m_is_error;
-        if (m_is_error)
+        if (m_is_error) {
             m_error = other.m_error;
-        else {
+        } else {
             if (other.m_have_storage) {
                 new (&m_storage) T(move(other.value()));
                 m_have_storage = true;