mw/com: Fix method signature to avoid copies of return type by bemerybmw · Pull Request #369 · eclipse-score/communication

bemerybmw · 2026-04-28T12:37:22Z

Depends-on: #514

github-actions · 2026-05-21T07:33:27Z

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed in 7 days if no further activity occurs. If this PR is still relevant, please leave a comment or push new changes to keep it open.

crimson11 · 2026-05-29T17:11:48Z

            constexpr bool is_return_type_not_void = !std::is_same_v<ReturnType, void>;
            if constexpr (is_return_type_not_void)
            {
+                const auto typed_return_ptr_tuple = Deserialize<ReturnType>(type_erased_return.value());


you could use DeserializeArg<ReturnType>() directly ... much simpler?

crimson11 · 2026-05-29T17:23:05Z

+    ::testing::Types<MethodTypeAndHandlerType<TestMethodType, TestMethodHandlerType>,
+                     MethodTypeAndHandlerType<void(int), void(const int&)>,
+                     MethodTypeAndHandlerType<void(int, int), void(const int&, const int&)>,
+                     MethodTypeAndHandlerType<void(const double, int), void(const double&, const int&)>,


Interessting. So you have a const in front of an in-arg .... in the user provided method-signature ... "technically" this is uneeded as with treat in-args as const anyhow! But here you are intentionally checking, that we also can cope with such a signature?

Yep, exactly.

crimson11 · 2026-05-29T17:30:54Z

@@ -99,17 +83,39 @@ TEST(SkeletonMethodTest, ClassTypeDependsOnMethodType)
                  "Class type does not depend on event data type");


event data type

method signature?

crimson11 · 2026-05-29T17:35:38Z

+    }
+
+    EmptySkeleton empty_skeleton_{std::make_unique<mock_binding::Skeleton>(), kInstanceIdWithLolaBinding};
+    std::string method_name_{"dummy_method"};


crimson11 · 2026-05-31T12:27:20Z


    template <typename T = SampleDataType, typename = std::enable_if_t<EnableSet && std::is_same_v<T, SampleDataType>>>
-    score::Result<MethodReturnTypePtr<T>> Set(SampleDataType& new_field_value) noexcept
+    score::Result<MethodReturnTypePtr<T>> Set(const SampleDataType& new_field_value) noexcept


Hm. Now I got aware, that we don't expose the zero-copy/allocate variant of method calls to the user ... I guess this would be just a small addition ... but let's wait til someone really asks for it :)

crimson11 · 2026-05-31T12:45:37Z

-        return a + b;
-    };
+    auto handler_with_in_args_and_return =
+        [](std::int32_t& return_value, const std::int32_t& a, const std::int32_t& b) {


maybe add a -> void ... makes it "clearer"?

KrishaDeshkool · 2026-06-01T14:56:44Z

        write_head += in_type_1_size;
        new (write_head) InType2(in_arg_2);
+        write_head += in_type_2_size;
+        new (write_head) InType2(in_arg_3);


the in_args_buffer_ is still sized with 2 elements here.
static constexpr std::size_t in_args_buffer_size = sizeof(InType1) + sizeof(InType2);
shouldn't this be
static constexpr std::size_t in_args_buffer_size = sizeof(InType1) + sizeof(InType2) + sizeof(InType2); ?

KrishaDeshkool · 2026-06-01T16:13:38Z

                    "ReturnType is non void. Thus, type_erased_result needs to have a value!");
-                ReturnType res = std::apply(callable_invoker, std::forward<InArgPtrTuple>(typed_in_arg_ptrs));
-                SerializeArgs<ReturnType>(type_erased_return.value(), res);
+                auto* const typed_return_ptr = DeserializeArg<ReturnType>(type_erased_return.value());


ah! we populate inplace, cool,
but, does it not weaken the "safe for trivially copyable" check?
in SerializeArgs route before we had this explicit check static_assert(std::is_trivially_copyable_v before we populate type_erased_return.
i think this is fine today though.

Hmmm, I actually don't think we have that requirement for deserializing. Deserializing is basically just casting the type, there's no copying done. In practice, we always serialize with the function in this file which already confirms that the type is trivially copyable but in theory, you could create a type erased buffer containing a non trivially copyable type (but the caller has to take that it's correctly created in the buffer) and then deserialize it without making a copy which should not be a problem. If the user then wants to copy it out of the buffer, it's now strongly typed and so the copy constructor can be called.

KrishaDeshkool · 2026-06-01T16:28:28Z

+{
+    detail::MemoryBufferAccessor memory_buffer_accessor{src_buffer};
+    auto tuple_of_args = detail::Deserialize<Arg>(memory_buffer_accessor);
+    SCORE_LANGUAGE_FUTURECPP_ASSERT_PRD_MESSAGE(std::tuple_size_v<decltype(tuple_of_args)> == 1,


this can be a static assert?

KrishaDeshkool · 2026-06-01T16:31:52Z

+                     MethodTypeAndHandlerType<void(int), void(const int&)>,
+                     MethodTypeAndHandlerType<void(int, int), void(const int&, const int&)>,
+                     MethodTypeAndHandlerType<void(const double, int), void(const double&, const int&)>,
+                     MethodTypeAndHandlerType<void(int, int), void(const int&, const int&)>,


This looks like a dup of https://github.com/eclipse-score/communication/pull/369/changes#diff-40f76737b28b11683a2795d7bc4658afc427cd2d105ba193b4f35133dc9fadbaR138
is this intended?

KrishaDeshkool · 2026-06-01T16:41:39Z

    Result<void> RegisterSetHandler(CallableType&& set_handler)
    {
-        static_assert(std::is_invocable_v<CallableType, FieldType&>,
+        static_assert(std::is_invocable_r_v<void, CallableType, FieldType&>,


does this really do something?
somehow, my static asserts here always work even when the ret type is non void.

KrishaDeshkool · 2026-06-01T16:43:12Z

-    score::cpp::callback<FixtureMethodType> test_callback{};
-
-    std::ignore = method.RegisterHandler(std::move(test_callback));
+    // // When a Register call is issued at the binding independent level


Suggested change

// // When a Register call is issued at the binding independent level

// When a Register call is issued at the binding independent level

KrishaDeshkool · 2026-06-11T09:23:41Z


-    ASSERT_TRUE(unit.my_setter_field_.Update(TestSampleType{1U}).has_value());
-    ASSERT_TRUE(unit.my_setter_field_.PrepareOffer().has_value());
+TEST_F(SkeletonFieldSetHandlerTest, CallingPrepareOfferWithoutRegisteringSetHandlerReturnsError)


isn't this the same test as here

communication/score/mw/com/impl/skeleton_field_test.cpp

Line 925 in c9c6438

TEST_F(SkeletonFieldSetHandlerTest, PrepareOfferFailsWhenSetHandlerNotRegistered)

KrishaDeshkool · 2026-06-11T10:32:39Z

-        return set_method_->RegisterHandler(std::move(wrapped_callback));
+
+        auto state = std::make_unique<std::pair<SkeletonField*, CallableType>>(
+            this, std::forward<CallableType>(user_set_handler));


wont this cause the same "use after free" issue?
since we store "this" during the registration into the stored set_handler and i dont think we update this when we move the SkeletonField then won't this still be pointing to the moved from object?
or am i missing something?

Yep, you're right. I wasn't trying to avoid the use after free issue here, it was rather about fitting all the state in the callable. I missed this point that we're on the binding independent layer in which the field is moveable. It's also a problem that we don't have a test that detects this...

KrishaDeshkool · 2026-06-11T10:43:41Z

+                      "Registered method callable must have a single non-const reference argument of type FieldType&!");
+
+        // Since set_handler can be an lvalue reference or an rvalue reference, we ideally would store it as a
+        // universal reference in the type_erased_handler. However, in C++17, this is not supported. Instead, we create


to me this sounds like this is C++17 only issue (storing a universal ref),
It is only used by compiler to figure out what is the type of the args, once it is figures that out it would be either an L or an R value, so can we just drop that confusing line ?
https://stackoverflow.com/questions/14757474/how-to-store-universal-references

This is no longer relevant here because I've removed the comment. But maybe it still applies to the comment in skeleton_method. But I don't fully understand your point, the issue with c++17 is not being able to "store" a universal reference in a lambda because we don't have templated lambdas. The link you sent is about "storing" a universal ref in a class. Probably easier if we just have a quick call and I can explain the problem / solution.

KrishaDeshkool · 2026-06-11T10:47:06Z

+        // Since set_handler can be an lvalue reference or an rvalue reference, we ideally would store it as a
+        // universal reference in the type_erased_handler. However, in C++17, this is not supported. Instead, we create
+        // an callable here which will be called below by another lambda which explicitly stores the callback as either
+        // an lvalue reference or an rvalue reference depending on how it was passed in.


does this also have lvalue vs rvalue branch like above? https://github.com/eclipse-score/communication/pull/369/changes#diff-5067b694039ba429e1f705db5d97521e3f0b3b0b07232117860ecd84ad9ebc80R212

This was a copy paste mistake. I was originally planning on using the same approach (with the if constexpr) but since I anyway need to put the state into a unique_ptr to fit it into the method handler (which is a score::cpp::callback of fixed size), we don't have the same problem (because the unique_ptr will either store an lvalue or rvalue reference depending on the template type of the CallableType).

crimson11 · 2026-06-12T16:57:07Z

+namespace score::mw::com::impl
+{
+
+/// \brief Factory that constructs a default-initialised CallableWrapper<HandlerSignature> via a static Create().


OK. Now I am NITPICKY. ;)

Factory that constructs a default-initialised CallableWrapper
better and shorter
Factory that default-constructs a CallableWrapper

The factory is calling the default-ctor here. Default-initializing is a broader process, where depending on the type, which is being default-initialized eventually also the default-ctor gets called

crimson11 · 2026-06-12T17:21:47Z

+/// - If `ReturnType` is `void` and there are no in arguments:
+///   `void()`
+template <FailureMode FailureMode, typename Callable, typename ReturnType, typename... ArgTypes>
+constexpr void AssertCallableMatchesMethodSignature()


Thankfully you had the extensive doc on top ;)
Because the name could be a bit misleading ...

AssertCallableMatchesMethodSignature
...
AssertMethodHandlerSupportsMethodSignature

... because matches (no native speaker ;) ) sounds like "equals" to me ... and Callable is so generic ... we exactly know, which callable type we talk about.

The merge-base changed after approval.

There still a review from Krishna that must be addressed.

bemerybmw added the invalid This doesn't seem right label Apr 28, 2026

bemerybmw force-pushed the brem_methods_dont_copy_return branch 2 times, most recently from 44e9aa3 to b336e39 Compare April 30, 2026 16:10

bemerybmw changed the title ~~Brem methods dont copy return~~ mw/com: Fix method signature to avoid copies of return type Apr 30, 2026

bemerybmw removed the invalid This doesn't seem right label Apr 30, 2026

github-actions Bot added the stale label May 21, 2026

bemerybmw force-pushed the brem_methods_dont_copy_return branch from b336e39 to cec7195 Compare May 27, 2026 15:38

bemerybmw commented May 27, 2026

View reviewed changes

Comment thread score/mw/com/impl/methods/method_handler_checker.h Outdated

github-actions Bot removed the stale label May 28, 2026

bemerybmw force-pushed the brem_methods_dont_copy_return branch from cec7195 to 5729f2f Compare May 28, 2026 09:38

bemerybmw marked this pull request as ready for review May 28, 2026 11:58

bemerybmw requested review from LittleHuba, castler, crimson11, hoe-jo and limdor as code owners May 28, 2026 11:58

crimson11 requested changes May 31, 2026

View reviewed changes

bemerybmw force-pushed the brem_methods_dont_copy_return branch 3 times, most recently from 4623c8f to 265b367 Compare June 1, 2026 12:48

KrishaDeshkool reviewed Jun 1, 2026

View reviewed changes

bemerybmw force-pushed the brem_methods_dont_copy_return branch 3 times, most recently from 804eec6 to beeff8d Compare June 9, 2026 13:59

bemerybmw added 4 commits June 9, 2026 16:31

mw/com: Add helpers for checking method handler signatures

a8aa11e

mw/com: Fix method signature to avoid copies of return type

1e44047

mw/com: Cleanup SkeletonField tests

c9c6438

mw/com: Add test that field set handler takes lvalue ref

cbc493d

bemerybmw force-pushed the brem_methods_dont_copy_return branch from beeff8d to cbc493d Compare June 9, 2026 14:34

KrishaDeshkool reviewed Jun 11, 2026

View reviewed changes

crimson11 previously approved these changes Jun 14, 2026

View reviewed changes

castler previously approved these changes Jun 15, 2026

View reviewed changes

castler closed this Jun 15, 2026

castler reopened this Jun 15, 2026

castler previously approved these changes Jun 15, 2026

View reviewed changes

bemerybmw marked this pull request as draft June 15, 2026 11:20

bemerybmw mentioned this pull request Jun 17, 2026

mw::com: Support for methods #302

Open

		@@ -99,17 +83,39 @@ TEST(SkeletonMethodTest, ClassTypeDependsOnMethodType)
		"Class type does not depend on event data type");

	// // When a Register call is issued at the binding independent level
	// When a Register call is issued at the binding independent level

Conversation

bemerybmw commented Apr 28, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions Bot commented May 21, 2026

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

KrishaDeshkool Jun 11, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

bemerybmw commented Apr 28, 2026 •

edited

Loading

KrishaDeshkool Jun 11, 2026 •

edited

Loading