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: #559
Depends-on: #572

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?

Completely reworked the tests.

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.

Replaced this with some more explicit checks using some helpers that I added.

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.

In the near future, we should implement similar tests for SkeletonFields and SkeletonMethods. We should also implement similar tests on proxy side.

In various places in our codebase, we have the issue in which we hand out a reference to a class which is moveable. If the class is moved, then we need to update this reference. E.g. SkeletonBase stores a map of references to its service elements. Each service element therefore must store a reference to the SkeletonBase and update the reference to itself when it's moved. The SkeletonBase must also update the reference to itself within each service element when it is moved. To avoid such complexity, this commit introduces a class ReferenceToMoveable which can be given to the SkeletonBase which is stored in the heap so a reference to it will never be invalidated. This then stores a reference to the service element which is updated by the service element if it's moved. With this approach, the service element doesn't need to store a reference to the parent SkeletonBase so the parent SkeletonBase doesn't need to inform the service element when it moves.

Previously, each service element had to store a reference to the parent SkeletonBase so that it could update the reference to itself in the SkeletonBase when the service element was moved. This meant that both SkeletonBase and the service elements had to store references to each other. Keeping these references in a valid state during moving was complex and error prone. We now use ReferenceToMoveableOwner in each service element which is updated when the service element moves. The SkeletonBase stores a ReferenceToMoveable to each service element, instead of a regular reference which is now always valid, regardless of moving of the SkeletonBase or service elements.

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 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

bemerybmw added 7 commits June 19, 2026 09:07

mw/com: Add integration tests for moving SkeletonEvent

0a8e7bc

In the near future, we should implement similar tests for SkeletonFields and SkeletonMethods. We should also implement similar tests on proxy side.

mw/com: Add helpers for checking method handler signatures

4c77771

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

d71b122

mw/com: Cleanup SkeletonField tests

ab0dc85

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

61caa6f

bemerybmw force-pushed the brem_methods_dont_copy_return branch from cbc493d to 61caa6f Compare June 19, 2026 09:25

github-project-automation Bot added this to COM - Communication FT Jun 19, 2026

github-project-automation Bot moved this to Backlog in COM - Communication FT Jun 19, 2026

bemerybmw added the test-qnx Enables the checks with QNX toolchains. Checks require committer approval to start execution. label Jun 19, 2026

		@@ -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!

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