Skip to content

Commit 3a96cdc

Browse files
committed
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. This commit: - Adds std::forward and std::move several places to preserve lvalue/rvalue status. - Defines a FunctionTraits::Fwd<...>(...) helper which does same thing as std::forward except it takes parameter number instead of a parameter type so generated code doesn't need as verbose as std::forward calls would be. - Changes C++ code generator to pass arguments as `M0::Fwd<1>(arg1)` instead of `arg1` in generated code. This change can be verified by diffing a generated file like build/src/ipc/capnp/mining.capnp.proxy-client.c++ in the bitcoin build before and after this change. Unlike the previous commit which resolved bugprone-move-forwarding-reference errors by just switching std::move to std::forward, this commit required more changes because just switching from std::move to std::forward in the type-interface.h CustomBuildField function would lead to another error in the CustomMakeProxyServer function there which is expecting an rvalue, not an lvalue. That error could have alternately been fixed by changing CustomMakeProxyServer to accept lvalues and copy the shared_ptr. But this would be slightly less efficient and it is better to resolve the problem at the root and just use perfect forwarding everywhere, all the way up the call stack. This required the changes to the code generator and clientInvoke described above.
1 parent c1e8c1a commit 3a96cdc

File tree

4 files changed

+35
-10
lines changed

4 files changed

+35
-10
lines changed

include/mp/proxy-types.h

Lines changed: 10 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -385,7 +385,7 @@ struct ClientException
385385
template <typename Accessor, typename... Types>
386386
struct ClientParam
387387
{
388-
ClientParam(Types&&... values) : m_values(values...) {}
388+
ClientParam(Types&&... values) : m_values{std::forward<Types>(values)...} {}
389389

390390
struct BuildParams : IterateFieldsHelper<BuildParams, sizeof...(Types)>
391391
{
@@ -399,7 +399,14 @@ struct ClientParam
399399
ParamList(), Priority<1>(), std::forward<Values>(values)..., Make<StructField, Accessor>(params));
400400
};
401401

402-
std::apply(fun, m_client_param->m_values);
402+
// Note: The m_values tuple just consists of lvalue and rvalue
403+
// references, so calling std::move doesn't change the tuple, it
404+
// just causes std::apply to call the std::get overload that returns
405+
// && instead of &, so rvalue references are preserved and not
406+
// turned into lvalue references. This allows the BuildField call to
407+
// move from the argument if it is an rvalue reference or was passed
408+
// by value.
409+
std::apply(fun, std::move(m_client_param->m_values));
403410
}
404411

405412
BuildParams(ClientParam* client_param) : m_client_param(client_param) {}
@@ -577,7 +584,7 @@ void serverDestroy(Server& server)
577584
//!
578585
//! ProxyClient<ClassName>::M0::Result ProxyClient<ClassName>::methodName(M0::Param<0> arg0, M0::Param<1> arg1) {
579586
//! typename M0::Result result;
580-
//! clientInvoke(*this, &InterfaceName::Client::methodNameRequest, MakeClientParam<...>(arg0), MakeClientParam<...>(arg1), MakeClientParam<...>(result));
587+
//! clientInvoke(*this, &InterfaceName::Client::methodNameRequest, MakeClientParam<...>(M0::Fwd<0>(arg0)), MakeClientParam<...>(M0::Fwd<1>(arg1)), MakeClientParam<...>(result));
581588
//! return result;
582589
//! }
583590
//!

include/mp/proxy.h

Lines changed: 12 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -181,7 +181,8 @@ struct ProxyServerCustom : public ProxyServerBase<Interface, Impl>
181181
//!
182182
//! Params - TypeList of C++ ClassName::methodName parameter types
183183
//! Result - Return type of ClassName::method
184-
//! Param<N> - helper to access individual parameters by index number.
184+
//! Param<N> - helper to access individual parameter types by index number.
185+
//! Fwd<N> - helper to forward arguments by index number.
185186
//! Fields - helper alias that appends Result type to the Params typelist if
186187
//! it not void.
187188
template <class Fn>
@@ -199,6 +200,16 @@ struct FunctionTraits<_Result (_Class::*const)(_Params...)>
199200
using Param = typename std::tuple_element<N, std::tuple<_Params...>>::type;
200201
using Fields =
201202
std::conditional_t<std::is_same_v<void, Result>, Params, TypeList<_Params..., _Result>>;
203+
204+
//! Enable perfect forwarding for clientInvoke calls. If parameter is a
205+
//! value type or rvalue reference type, pass it as an rvalue-reference to
206+
//! MakeClientParam and BuildField calls so it can be moved from, and if it
207+
//! is an lvalue reference, pass it an lvalue reference so it won't be moved
208+
//! from. This method does the same thing as std::forward except it takes a
209+
//! parameter number instead of a type as a template argument, so generated
210+
//! code calling this can be less repetitive and verbose.
211+
template <size_t N>
212+
static decltype(auto) Fwd(Param<N>& arg) { return static_cast<Param<N>&&>(arg); }
202213
};
203214

204215
//! Traits class for a proxy method, providing the same

include/mp/type-interface.h

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -44,7 +44,7 @@ void CustomBuildField(TypeList<std::shared_ptr<Impl>>,
4444
{
4545
if (value) {
4646
using Interface = typename decltype(output.get())::Calls;
47-
output.set(CustomMakeProxyServer<Interface, Impl>(invoke_context, std::move(value)));
47+
output.set(CustomMakeProxyServer<Interface, Impl>(invoke_context, std::forward<Value>(value)));
4848
}
4949
}
5050

src/mp/gen.cpp

Lines changed: 12 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -512,10 +512,20 @@ static void Generate(kj::StringPtr src_prefix,
512512

513513
add_accessor(field_name);
514514

515+
std::ostringstream fwd_args;
515516
for (int i = 0; i < field.args; ++i) {
516517
if (argc > 0) client_args << ",";
518+
519+
// Add to client method parameter list.
517520
client_args << "M" << method_ordinal << "::Param<" << argc << "> " << field_name;
518521
if (field.args > 1) client_args << i;
522+
523+
// Add to MakeClientParam argument list using Fwd helper for perfect forwarding.
524+
if (i > 0) fwd_args << ", ";
525+
fwd_args << "M" << method_ordinal << "::Fwd<" << argc << ">(" << field_name;
526+
if (field.args > 1) fwd_args << i;
527+
fwd_args << ")";
528+
519529
++argc;
520530
}
521531
client_invoke << ", ";
@@ -529,13 +539,10 @@ static void Generate(kj::StringPtr src_prefix,
529539
client_invoke << "Accessor<" << base_name << "_fields::" << Cap(field_name) << ", "
530540
<< field_flags.str() << ">>(";
531541

532-
if (field.retval || field.args == 1) {
542+
if (field.retval) {
533543
client_invoke << field_name;
534544
} else {
535-
for (int i = 0; i < field.args; ++i) {
536-
if (i > 0) client_invoke << ", ";
537-
client_invoke << field_name << i;
538-
}
545+
client_invoke << fwd_args.str();
539546
}
540547
client_invoke << ")";
541548

0 commit comments

Comments
 (0)