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

This is the copy variant of set which takes a field value and returns the new value via the MethodReturnTypePtr. The provided value will not be modified so it's misleading that it's non-const.

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?

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

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

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 added 3 commits June 9, 2026 08:18

mw/com: Rename Deserialize to match SerializeArgs

37adcf6

mw/com: Add DeserializeArg helper

957b050

mw/com: Extract boilerplate into fixture

88e5034

bemerybmw added 3 commits June 9, 2026 15:28

mw/com: Make input argument to field Set const

f3ccb00

This is the copy variant of set which takes a field value and returns the new value via the MethodReturnTypePtr. The provided value will not be modified so it's misleading that it's non-const.

mw/com: Fix naming

b5a11a0

mw/com: Use nice mocks to avoid spam in test logs

cb57fb6

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

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

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

bemerybmw commented Apr 28, 2026 •

edited

Loading

KrishaDeshkool Jun 11, 2026 •

edited

Loading