From 154af1eea1170f5626aa1c5f19cc77d1434bcc9d Mon Sep 17 00:00:00 2001 From: Ryan Ofsky Date: Thu, 29 May 2025 13:57:08 -0400 Subject: [PATCH] Squashed 'src/ipc/libmultiprocess/' changes from 35944ffd23fa..27c7e8e5a581 27c7e8e5a581 Merge chaincodelabs/libmultiprocess#172: refactor: fix warnings from clang-tidy-20 and bitcoin-tidy 2fe87d016be4 Merge chaincodelabs/libmultiprocess#173: doc: Fix error string typo 57a65b854664 clang-tidy: Suppress bitcoin-nontrivial-threadlocal error 0d8012f656fe Merge chaincodelabs/libmultiprocess#165: clang-tidy: fix warnings introduced in version 19 3a96cdc18f2d clang-tidy: Fix bugprone-move-forwarding-reference error c1e8c1a02864 clang-tidy: Fix bugprone-move-forwarding-reference errors aa19285303ff use ranges transform a78137ca73b8 make member function const ca3226ec8ab7 replace custom tuple unpacking code with `std::apply` 949fe85fc91f replace SFINAE trick with `if constexpr` 44ee4b40b89a doc: Fix error string typo git-subtree-dir: src/ipc/libmultiprocess git-subtree-split: 27c7e8e5a581b3c41330e758951251ef11807b11 --- include/mp/proxy-io.h | 11 +-- include/mp/proxy-types.h | 137 +++++++++++++++++++----------------- include/mp/proxy.h | 13 +++- include/mp/type-interface.h | 2 +- include/mp/util.h | 2 +- src/mp/gen.cpp | 19 +++-- src/mp/proxy.cpp | 2 +- src/mp/util.cpp | 2 +- 8 files changed, 107 insertions(+), 81 deletions(-) diff --git a/include/mp/proxy-io.h b/include/mp/proxy-io.h index 4eb27fae7c4..dff8c2a63a4 100644 --- a/include/mp/proxy-io.h +++ b/include/mp/proxy-io.h @@ -172,7 +172,7 @@ public: void addClient(std::unique_lock& lock); bool removeClient(std::unique_lock& lock); //! Check if loop should exit. - bool done(std::unique_lock& lock); + bool done(std::unique_lock& lock) const; Logger log() { @@ -249,7 +249,7 @@ struct Waiter { const std::unique_lock lock(m_mutex); assert(!m_fn); - m_fn = std::move(fn); + m_fn = std::forward(fn); m_cv.notify_all(); } @@ -333,7 +333,7 @@ public: // 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), 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 diff --git a/include/mp/proxy-types.h b/include/mp/proxy-types.h index 1a519efde02..a74c6de0b99 100644 --- a/include/mp/proxy-types.h +++ b/include/mp/proxy-types.h @@ -37,20 +37,45 @@ struct StructField } Struct& m_struct; - // clang-format off - template auto get() const -> decltype(A::get(this->m_struct)) { return A::get(this->m_struct); } - template auto has() const -> std::enable_if_t { return A::getHas(m_struct); } - template auto has() const -> std::enable_if_t { return A::has(m_struct); } - template auto has() const -> std::enable_if_t { return true; } - template auto want() const -> std::enable_if_t { return A::getWant(m_struct); } - template auto want() const -> std::enable_if_t { return true; } - template decltype(auto) set(Args&&... args) const { return A::set(this->m_struct, std::forward(args)...); } - template decltype(auto) init(Args&&... args) const { return A::init(this->m_struct, std::forward(args)...); } - template auto setHas() const -> std::enable_if_t { return A::setHas(m_struct); } - template auto setHas() const -> std::enable_if_t { } - template auto setWant() const -> std::enable_if_t { return A::setWant(m_struct); } - template auto setWant() const -> std::enable_if_t { } - // clang-format on + decltype(auto) get() const { return Accessor::get(this->m_struct); } + + bool has() const { + if constexpr (Accessor::optional) { + return Accessor::getHas(m_struct); + } else if constexpr (Accessor::boxed) { + return Accessor::has(m_struct); + } else { + return true; + } + } + + bool want() const { + if constexpr (Accessor::requested) { + return Accessor::getWant(m_struct); + } else { + return true; + } + } + + template decltype(auto) set(Args &&...args) const { + return Accessor::set(this->m_struct, std::forward(args)...); + } + + template decltype(auto) init(Args &&...args) const { + return Accessor::init(this->m_struct, std::forward(args)...); + } + + void setHas() const { + if constexpr (Accessor::optional) { + Accessor::setHas(m_struct); + } + } + + void setWant() const { + if constexpr (Accessor::requested) { + Accessor::setWant(m_struct); + } + } }; @@ -360,34 +385,28 @@ struct ClientException template struct ClientParam { - ClientParam(Types&&... values) : m_values(values...) {} + ClientParam(Types&&... values) : m_values{std::forward(values)...} {} struct BuildParams : IterateFieldsHelper { - template - void handleField(Args&&... args) + template + void handleField(ClientInvokeContext& invoke_context, Params& params, ParamList) { - callBuild<0>(std::forward(args)...); - } + auto const fun = [&](Values&&... values) { + MaybeBuildField(std::integral_constant(), ParamList(), invoke_context, + Make(params), std::forward(values)...); + MaybeSetWant( + ParamList(), Priority<1>(), std::forward(values)..., Make(params)); + }; - // TODO Possible optimization to speed up compile time: - // https://stackoverflow.com/a/7858971 Using enable_if below to check - // position when unpacking tuple might be slower than pattern matching - // approach in the stack overflow solution - template - auto callBuild(Args&&... args) -> std::enable_if_t<(I < sizeof...(Types))> - { - callBuild(std::forward(args)..., std::get(m_client_param->m_values)); - } - - template - auto callBuild(ClientInvokeContext& invoke_context, Params& params, ParamList, Values&&... values) -> - std::enable_if_t<(I == sizeof...(Types))> - { - MaybeBuildField(std::integral_constant(), ParamList(), invoke_context, - Make(params), std::forward(values)...); - MaybeSetWant( - ParamList(), Priority<1>(), std::forward(values)..., Make(params)); + // 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) {} @@ -396,24 +415,15 @@ struct ClientParam struct ReadResults : IterateFieldsHelper { - template - void handleField(Args&&... args) + template + void handleField(ClientInvokeContext& invoke_context, Results& results, TypeList) { - callRead<0>(std::forward(args)...); - } + auto const fun = [&](Values&&... values) { + MaybeReadField(std::integral_constant(), TypeList...>(), invoke_context, + Make(results), ReadDestUpdate(values)...); + }; - template - auto callRead(Args&&... args) -> std::enable_if_t<(I < sizeof...(Types))> - { - callRead(std::forward(args)..., std::get(m_client_param->m_values)); - } - - template - auto callRead(ClientInvokeContext& invoke_context, Results& results, TypeList, Values&&... values) - -> std::enable_if_t - { - MaybeReadField(std::integral_constant(), TypeList...>(), invoke_context, - Make(results), ReadDestUpdate(values)...); + std::apply(fun, m_client_param->m_values); } ReadResults(ClientParam* client_param) : m_client_param(client_param) {} @@ -574,7 +584,7 @@ void serverDestroy(Server& server) //! //! ProxyClient::M0::Result ProxyClient::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; //! } //! @@ -650,19 +660,14 @@ void clientInvoke(ProxyClient& proxy_client, const GetRequest& get_request, Fiel //! return value with value of `ret()`. This is useful for avoiding code //! duplication and branching in generic code that forwards calls to functions. template -auto ReplaceVoid(Fn&& fn, Ret&& ret) -> - std::enable_if_t, decltype(ret())> +auto ReplaceVoid(Fn&& fn, Ret&& ret) { - fn(); - return ret(); -} - -//! Overload of above for non-void `fn()` case. -template -auto ReplaceVoid(Fn&& fn, Ret&& ret) -> - std::enable_if_t, decltype(fn())> -{ - return fn(); + if constexpr (std::is_same_v) { + fn(); + return ret(); + } else { + return fn(); + } } extern std::atomic server_reqs; diff --git a/include/mp/proxy.h b/include/mp/proxy.h index 76be0992109..e7faad9a666 100644 --- a/include/mp/proxy.h +++ b/include/mp/proxy.h @@ -181,7 +181,8 @@ struct ProxyServerCustom : public ProxyServerBase //! //! Params - TypeList of C++ ClassName::methodName parameter types //! Result - Return type of ClassName::method -//! Param - helper to access individual parameters by index number. +//! Param - helper to access individual parameter types by index number. +//! Fwd - helper to forward arguments by index number. //! Fields - helper alias that appends Result type to the Params typelist if //! it not void. template @@ -199,6 +200,16 @@ struct FunctionTraits<_Result (_Class::*const)(_Params...)> using Param = typename std::tuple_element>::type; using Fields = std::conditional_t, 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 + static decltype(auto) Fwd(Param& arg) { return static_cast&&>(arg); } }; //! Traits class for a proxy method, providing the same diff --git a/include/mp/type-interface.h b/include/mp/type-interface.h index 99adf2ab94e..8a89ac24fca 100644 --- a/include/mp/type-interface.h +++ b/include/mp/type-interface.h @@ -44,7 +44,7 @@ void CustomBuildField(TypeList>, { if (value) { using Interface = typename decltype(output.get())::Calls; - output.set(CustomMakeProxyServer(invoke_context, std::move(value))); + output.set(CustomMakeProxyServer(invoke_context, std::forward(value))); } } diff --git a/include/mp/util.h b/include/mp/util.h index ebfc3b5e720..8b802abc9f3 100644 --- a/include/mp/util.h +++ b/include/mp/util.h @@ -183,7 +183,7 @@ struct AsyncCallable template AsyncCallable> MakeAsyncCallable(Callable&& callable) { - return std::move(callable); + return std::forward(callable); } //! Format current thread name as "{exe_name}-{$pid}/{thread_name}-{$tid}". diff --git a/src/mp/gen.cpp b/src/mp/gen.cpp index 267c2837a66..3d841a3f5e5 100644 --- a/src/mp/gen.cpp +++ b/src/mp/gen.cpp @@ -215,7 +215,7 @@ static void Generate(kj::StringPtr src_prefix, cpp_types << "namespace mp {\n"; std::string guard = output_path; - std::transform(guard.begin(), guard.end(), guard.begin(), [](unsigned char c) -> unsigned char { + std::ranges::transform(guard, guard.begin(), [](unsigned char c) -> unsigned char { if ('0' <= c && c <= '9') return c; if ('A' <= c && c <= 'Z') return c; if ('a' <= c && c <= 'z') return c - 'a' + 'A'; @@ -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 << ")"; diff --git a/src/mp/proxy.cpp b/src/mp/proxy.cpp index 194ded5f265..b4255b60fea 100644 --- a/src/mp/proxy.cpp +++ b/src/mp/proxy.cpp @@ -277,7 +277,7 @@ void EventLoop::startAsyncThread(std::unique_lock& lock) } } -bool EventLoop::done(std::unique_lock& lock) +bool EventLoop::done(std::unique_lock& lock) const { assert(m_num_clients >= 0); assert(lock.owns_lock()); diff --git a/src/mp/util.cpp b/src/mp/util.cpp index 691ae0b34b5..309bb922352 100644 --- a/src/mp/util.cpp +++ b/src/mp/util.cpp @@ -137,7 +137,7 @@ void ExecProcess(const std::vector& args) } argv.push_back(nullptr); if (execvp(argv[0], argv.data()) != 0) { - perror("execlp failed"); + perror("execvp failed"); _exit(1); } }