clang-tidy: Fix bugprone-move-forwarding-reference error

/ci_container_base/include/mp/type-interface.h:47:75: error: forwarding reference passed to std::move(), which may unexpectedly cause lvalues to be moved; use std::forward() instead [bugprone-move-forwarding-reference,-warnings-as-errors]
https://cirrus-ci.com/task/6187773452877824?logs=ci#L4712

Error was caused by mp/type-interface.h CustomBuildField field using std::move
instead of std::forward.

Just switching from std::move to std::forward, however, would lead to another
error in the CustomMakeProxyServer function which is expecting an rvalue, not
an lvalue.

That error could be fixed by changing CustomMakeProxyServer to accept lvalues
and copy the shared_ptr, which would have a slight performance cost. But it is
better to resolve the problem at the root and switch to perfect forwarding
everywhere, all the way up the call stack. This required some small changes to
the code generator and clientInvoke.

Author: ryanofsky

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,14 @@ 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.
+    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)));
     }
 }
 

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 << ")";