Merge #172: refactor: fix warnings from clang-tidy-20 and bitcoin-tidy

57a65b8 clang-tidy: Suppress bitcoin-nontrivial-threadlocal error (Ryan Ofsky)
3a96cdc clang-tidy: Fix bugprone-move-forwarding-reference error (Ryan Ofsky)
c1e8c1a clang-tidy: Fix bugprone-move-forwarding-reference errors (Ryan Ofsky)

Pull request description:

  Warnings are described in commit messages and were reported by Sjors in bitcoin/bitcoin#31802 (comment)

ACKs for top commit:
  Sjors:
    ACK 57a65b8
  TheCharlatan:
    ACK 57a65b8

Tree-SHA512: 3018126aa174f3c1d11abe5a5601c5c3fdee49b4a6ca4c19abd766eea88ba765d5a0f942dd28db5db9332256fb3162c9638615004e1348d0d2687d9b54dab2cc

Author: ryanofsky

include/mp/proxy-io.h

@@ -249,7 +249,7 @@ struct Waiter
     {
         const std::unique_lock<std::mutex> lock(m_mutex);
         assert(!m_fn);
-        m_fn = std::move(fn);
+        m_fn = std::forward<Fn>(fn);
         m_cv.notify_all();
     }
 
@@ -333,7 +333,7 @@ class Connection
         // to the EventLoop TaskSet to avoid "Promise callback destroyed itself"
         // error in cases where f deletes this Connection object.
         m_on_disconnect.add(m_network.onDisconnect().then(
-            [f = std::move(f), this]() mutable { m_loop.m_task_set->add(kj::evalLater(kj::mv(f))); }));
+            [f = std::forward<F>(f), this]() mutable { m_loop.m_task_set->add(kj::evalLater(kj::mv(f))); }));
     }
 
     EventLoop& m_loop;
@@ -634,7 +634,10 @@ void ListenConnections(EventLoop& loop, int fd, InitImpl& init)
     });
 }
 
-extern thread_local ThreadContext g_thread_context;
+extern thread_local ThreadContext g_thread_context; // NOLINT(bitcoin-nontrivial-threadlocal)
+// Silence nonstandard bitcoin tidy error "Variable with non-trivial destructor
+// cannot be thread_local" which should not be a problem on modern platforms, and
+// could lead to a small memory leak at worst on older ones.
 
 } // namespace mp
 

include/mp/proxy-types.h

@@ -385,7 +385,7 @@ struct ClientException
 template <typename Accessor, typename... Types>
 struct ClientParam
 {
-    ClientParam(Types&&... values) : m_values(values...) {}
+    ClientParam(Types&&... values) : m_values{std::forward<Types>(values)...} {}
 
     struct BuildParams : IterateFieldsHelper<BuildParams, sizeof...(Types)>
     {
@@ -399,7 +399,14 @@ struct ClientParam
                     ParamList(), Priority<1>(), std::forward<Values>(values)..., Make<StructField, Accessor>(params));
             };
 
-            std::apply(fun, m_client_param->m_values);
+            // Note: The m_values tuple just consists of lvalue and rvalue
+            // references, so calling std::move doesn't change the tuple, it
+            // just causes std::apply to call the std::get overload that returns
+            // && instead of &, so rvalue references are preserved and not
+            // turned into lvalue references. This allows the BuildField call to
+            // move from the argument if it is an rvalue reference or was passed
+            // by value.
+            std::apply(fun, std::move(m_client_param->m_values));
         }
 
         BuildParams(ClientParam* client_param) : m_client_param(client_param) {}
@@ -577,7 +584,7 @@ void serverDestroy(Server& server)
 //!
 //! ProxyClient<ClassName>::M0::Result ProxyClient<ClassName>::methodName(M0::Param<0> arg0, M0::Param<1> arg1) {
 //!     typename M0::Result result;
-//!     clientInvoke(*this, &InterfaceName::Client::methodNameRequest, MakeClientParam<...>(arg0), MakeClientParam<...>(arg1), MakeClientParam<...>(result));
+//!     clientInvoke(*this, &InterfaceName::Client::methodNameRequest, MakeClientParam<...>(M0::Fwd<0>(arg0)), MakeClientParam<...>(M0::Fwd<1>(arg1)), MakeClientParam<...>(result));
 //!     return result;
 //! }
 //!

include/mp/proxy.h

@@ -181,7 +181,8 @@ struct ProxyServerCustom : public ProxyServerBase<Interface, Impl>
 //!
 //! Params   - TypeList of C++ ClassName::methodName parameter types
 //! Result   - Return type of ClassName::method
-//! Param<N> - helper to access individual parameters by index number.
+//! Param<N> - helper to access individual parameter types by index number.
+//! Fwd<N>   - helper to forward arguments by index number.
 //! Fields   - helper alias that appends Result type to the Params typelist if
 //!            it not void.
 template <class Fn>
@@ -199,6 +200,16 @@ struct FunctionTraits<_Result (_Class::*const)(_Params...)>
     using Param = typename std::tuple_element<N, std::tuple<_Params...>>::type;
     using Fields =
         std::conditional_t<std::is_same_v<void, Result>, Params, TypeList<_Params..., _Result>>;
+
+    //! Enable perfect forwarding for clientInvoke calls. If parameter is a
+    //! value type or rvalue reference type, pass it as an rvalue-reference to
+    //! MakeClientParam and BuildField calls so it can be moved from, and if it
+    //! is an lvalue reference, pass it an lvalue reference so it won't be moved
+    //! from. This method does the same thing as std::forward except it takes a
+    //! parameter number instead of a type as a template argument, so generated
+    //! code calling this can be less repetitive and verbose.
+    template <size_t N>
+    static decltype(auto) Fwd(Param<N>& arg) { return static_cast<Param<N>&&>(arg); }
 };
 
 //! Traits class for a proxy method, providing the same

include/mp/type-interface.h

@@ -44,7 +44,7 @@ void CustomBuildField(TypeList<std::shared_ptr<Impl>>,
 {
     if (value) {
         using Interface = typename decltype(output.get())::Calls;
-        output.set(CustomMakeProxyServer<Interface, Impl>(invoke_context, std::move(value)));
+        output.set(CustomMakeProxyServer<Interface, Impl>(invoke_context, std::forward<Value>(value)));
     }
 }
 

include/mp/util.h

@@ -183,7 +183,7 @@ struct AsyncCallable
 template <typename Callable>
 AsyncCallable<std::remove_reference_t<Callable>> MakeAsyncCallable(Callable&& callable)
 {
-    return std::move(callable);
+    return std::forward<Callable>(callable);
 }
 
 //! Format current thread name as "{exe_name}-{$pid}/{thread_name}-{$tid}".

src/mp/gen.cpp

@@ -512,10 +512,20 @@ static void Generate(kj::StringPtr src_prefix,
 
                     add_accessor(field_name);
 
+                    std::ostringstream fwd_args;
                     for (int i = 0; i < field.args; ++i) {
                         if (argc > 0) client_args << ",";
+
+                        // Add to client method parameter list.
                         client_args << "M" << method_ordinal << "::Param<" << argc << "> " << field_name;
                         if (field.args > 1) client_args << i;
+
+                        // Add to MakeClientParam argument list using Fwd helper for perfect forwarding.
+                        if (i > 0) fwd_args << ", ";
+                        fwd_args << "M" << method_ordinal << "::Fwd<" << argc << ">(" << field_name;
+                        if (field.args > 1) fwd_args << i;
+                        fwd_args << ")";
+
                         ++argc;
                     }
                     client_invoke << ", ";
@@ -529,13 +539,10 @@ static void Generate(kj::StringPtr src_prefix,
                     client_invoke << "Accessor<" << base_name << "_fields::" << Cap(field_name) << ", "
                                   << field_flags.str() << ">>(";
 
-                    if (field.retval || field.args == 1) {
+                    if (field.retval) {
                         client_invoke << field_name;
                     } else {
-                        for (int i = 0; i < field.args; ++i) {
-                            if (i > 0) client_invoke << ", ";
-                            client_invoke << field_name << i;
-                        }
+                        client_invoke << fwd_args.str();
                     }
                     client_invoke << ")";