Add field tags by KrishaDeshkool · Pull Request #357 · eclipse-score/communication

KrishaDeshkool · 2026-04-24T13:12:52Z

No description provided.

KrishaDeshkool · 2026-04-27T08:53:21Z

Once the CI is green, i will make this PR open and can we reviewed.

KrishaDeshkool · 2026-05-04T14:41:09Z

+    /// ctors via the detail::WithTestTag marker, required as the 3rd argument. Bindings default to nullptr;
    ProxyField(ProxyBase& proxy_base,
               const std::string_view field_name,
+               detail::WithTestTag,


This is the topic i wanted to discuss,
if we do not have this Tag then if we make all other bindings as nullptr, in other words make these defaults,
then i have the following issue,
when all the bingngs are nullptr then the all of them are defaulted and depending on the tag combination , the real production ctr would clash with this test ctr and since the prod ctr is templated the compiler would pick this non templated ctr and hence we run into issues in production, this is why i have introduced this WithTestTag to clearly say this is test only ctr, i think it also reads much better than adding a comment on the ctr that this is a test only ctr.

See comment here: #357 (comment)

The new order allows us to set the bindings to nullptrs by default for test only constructors. This will be useful for fields which will require optionally inserting get / set bindings. We change all constructors so that they're all consistent.

- Use a single test constructor which creates the get / set methods according to the provided bindings. This allows us to remove the test-only constructor overloads for each template arg combination. - Always store the get / set method dispatches as unique_ptrs even if they're not enabled (when disabled, they'll simply be nullptrs). This allows us to have a single private constructor which always accepts unique_ptrs (which may be valid or nullptrs).

Replaces the EnableGet/EnableSet/EnableNotifier bool template parameters on ProxyField and SkeletonField with a variadic pack of tags (WithGetter, WithSetter) declared in field_tags.h. Presence of a tag enables the matching API. The notifier surface stays unconditional on both sides, since a field without a notifier cannot propagate value changes. Also updates friend/forward declarations in ProxyEvent, ProxyMethod, SkeletonEvent and SkeletonMethod, the Trait::Field aliases plus the traits example doc, and the skeleton_field_test.cpp call sites.

* Introducing a WithNotifier field tag * When both getter and notification mechanism is turned off there is no way for the consumer to observe the field values. making this configuration a compile time failure.

crimson11 · 2026-05-05T11:01:53Z

        .WillOnce(Return(ByMove(std::move(skeleton_field_binding_mock_ptr))));
-    // Field get method binding (not yet fully implemented; set method only exists when EnableSet=true)
+    // Field set method binding (only created when the SkeletonField is tagged WithSetter)
    EXPECT_CALL(skeleton_method_binding_factory_mock_guard_.factory_mock_, Create(_, _, _, _))


You should be more concise here? You are expecting a call to Create with MethodType kSet? ... and eventually also test/make clear, what "method-name" we are expecting here? Hint - in this case the method-name is the field-name afaik ...

I guess - currently gtest is smart enough to correctly assign the EXPECT in line 553 vs line 559 by the arg kMethodName ...

bemerybmw · 2026-05-07T10:38:26Z

+// parameters. The std::is_same<FieldType, ClassFieldType> line ties substitution to a function-template
+// parameter so the check fires at overload resolution instead of class instantiation.
+template <typename FieldType, typename ClassFieldType, typename TargetTag, typename... Tags>
+struct is_tag_enabled : std::conjunction<contains_type<TargetTag, Tags...>, std::is_same<FieldType, ClassFieldType>>


Since is_tag_enabled is a public function to be used by other targets, it shouldn't be in the detail namespace.

bemerybmw · 2026-05-07T10:40:44Z

+// parameter so the check fires at overload resolution instead of class instantiation.
+template <typename FieldType, typename ClassFieldType, typename TargetTag, typename... Tags>
+struct is_tag_enabled : std::conjunction<contains_type<TargetTag, Tags...>, std::is_same<FieldType, ClassFieldType>>
+{


We should have unit tests for any public function (so definitely is_tag_enabled and if contains_type is publicly exposed i.e. not in the detail namespace then also for it). You can look at the unit tests that I wrote for method_handler_checker_test as a reference if you want: https://github.com/eclipse-score/communication/pull/369/changes#diff-a5298ded87ed55bcf9aceb680edbe7e0c9b81700b7a5e0f24feb7b3600dac64d

We can only support "positive" tests for the moment, but you should also write the negative tests e.g.

communication/score/mw/com/impl/methods/method_handler_checker_test.cpp

Line 41 in b336e39

#if 0

bemerybmw · 2026-05-07T10:42:40Z

-          const bool EnableSet = false,
-          const bool EnableNotifier = false>
+/// \tparam Tags Optional tag pack; presence of WithGetter / WithSetter enables Get() / Set() respectively.
+///              The notifier surface (Subscribe, GetNewSamples, ...) is part of every field.


What does "surface" mean here?

bemerybmw · 2026-05-07T10:44:17Z

-          const bool EnableNotifier = false>
+/// \tparam Tags Optional tag pack; presence of WithGetter / WithSetter enables Get() / Set() respectively.
+///              The notifier surface (Subscribe, GetNewSamples, ...) is part of every field.
+template <typename SampleDataType, typename... Tags>


Maybe be explicit in the documentation that the supported tags are any combination of the tags defined in field_tags.h (i.e. WithGetter, WithSetter, WithNotifier)

bemerybmw · 2026-05-07T10:45:33Z

-    ///
-    /// This is used for testing only. Allows for directly setting the bindings, and usually the mock binding is used
-    /// here.
+    /// Testing ctor: bindings are passed in directly (used with mock bindings).


Please use the doxygen format.

/// \brief ... /// /// More details here.

bemerybmw · 2026-05-07T11:36:46Z


-    /// Testing ctor: bindings are passed in directly (used with mock bindings).
+    /// Testing ctor: bindings are passed in directly (used with mock bindings). The event binding is required
+    /// (no default) to disambiguate this overload from the production ctors when called as `{proxy, field_name}`.


But there is a default agrument for event_binding?

bemerybmw · 2026-05-07T11:38:34Z

-              proxy_base,
-              field_name,
-              std::make_unique<ProxyEvent<FieldType>>(proxy_base, field_name, std::move(event_binding)),
-              // If a binding is not provided, then we don't create the method. This ensures that the ProxyMethod


We need to document the seond sentence "This ensures that the ProxyMethod doesn't report that the binding is invalid (via proxy_base_view.MarkServiceElementBindingInvalid())" somewhere. Either here or in the docstring for the function.

bemerybmw · 2026-05-07T11:44:16Z

+    static std::unique_ptr<ProxyMethod<FieldType()>> MakeGetMethodDispatchFromFactory(ProxyBase& proxy_base,
+                                                                                      const std::string_view field_name)
+    {
+        if constexpr (kHasGetter)


I think it would be clearer to only call this function when a getter is enabled. I.e. the construtor either calls MakeGetMethodDispatchFromFactory or explicitly passes a nullptr. That way it's clear from the constructor whether a method dispatch is created or not.

On second thought, what could make more sense is to have one production constructor and the dispatch objects are created according to the tags in these "Make" functions. Because currently all those constructors do the same thing, i.e. simply dispatch to these functions. In this case, maybe you can rename them to MakeGetMethodDispatchIfUsed or MakeGetMethodDispatchIfEnabled or something.

bemerybmw · 2026-05-07T11:50:09Z

    using ProxyEventFieldMock = typename T::ProxyEventFieldMock;

    ProxyEventFieldMockFixture()
+        : unit_{[this] {


I don't think this commit makes sense. I don't think we should add a tag for a test only "helper" constructor. I think we just live with the limitation that we have to always provide the event binding (either with a real binding or a nullptr).

bemerybmw · 2026-05-07T11:53:07Z

        .WillOnce(Return(ByMove(std::move(skeleton_field_binding_mock_ptr))));
-    // Field set method binding (only created when the SkeletonField is tagged WithSetter)
-    EXPECT_CALL(skeleton_method_binding_factory_mock_guard_.factory_mock_, Create(_, _, _, _))
+    // Field get method binding always created for every SkeletonField regardless of tags, because get_method_ is


This test is anyway fixed by https://github.com/eclipse-score/communication/pull/373/changes#diff-412e28d5d756f0c6692791281ab7995b8b0ec4935a6d836fd68ee6cdb71a2b7e. So I think you can remove this commit.

KrishaDeshkool force-pushed the add_field_tags branch 6 times, most recently from 323edba to 278adac Compare April 27, 2026 07:44

KrishaDeshkool mentioned this pull request Apr 27, 2026

mw:com: add missing coverage to get set feature #350

Draft

KrishaDeshkool force-pushed the add_field_tags branch from 278adac to c034c1e Compare April 27, 2026 08:50

KrishaDeshkool force-pushed the add_field_tags branch from c034c1e to 894a5e7 Compare April 27, 2026 08:53

KrishaDeshkool marked this pull request as ready for review April 27, 2026 09:40

KrishaDeshkool requested review from LittleHuba, bemerybmw, castler, crimson11, hoe-jo and limdor as code owners April 27, 2026 09:40

KrishaDeshkool marked this pull request as draft April 28, 2026 07:34

crimson11 requested changes Apr 28, 2026

View reviewed changes

Comment thread score/mw/com/impl/skeleton_field.h Outdated

Comment thread score/mw/com/impl/skeleton_field.h

KrishaDeshkool force-pushed the add_field_tags branch 7 times, most recently from e47f755 to a4a564f Compare May 4, 2026 14:18

KrishaDeshkool commented May 4, 2026

View reviewed changes

KrishaDeshkool force-pushed the add_field_tags branch from a1b5a44 to e0feeca Compare May 4, 2026 15:04

KrishaDeshkool added 2 commits May 5, 2026 11:02

KrishaDeshkool force-pushed the add_field_tags branch 4 times, most recently from 50abfa1 to 879c9ce Compare May 5, 2026 09:29

KrishaDeshkool added 7 commits May 5, 2026 11:32

mw/com: add WithNotifier field tag and enforce observable conditions

c45d01a

* Introducing a WithNotifier field tag * When both getter and notification mechanism is turned off there is no way for the consumer to observe the field values. making this configuration a compile time failure.

mw/com: SFINAE-gate proxy field notifier surface on WithNotifier

5a4f33f

mw/com: add SFINAE detection tests for proxy field notifier surface

1337ff1

mw/com: make event_binding defaulted to nullptr

915ef07

mw/com: expand proxy field ctors to one per tag combination

4ccf76c

mw/com: disambiguate proxy field test ctor with WithTestTag

47c26d9

KrishaDeshkool force-pushed the add_field_tags branch from 879c9ce to 47c26d9 Compare May 5, 2026 09:32

KrishaDeshkool marked this pull request as ready for review May 5, 2026 09:32

crimson11 requested changes May 5, 2026

View reviewed changes

mw::com: make expectations more explicit

44de0b0

KrishaDeshkool requested a review from crimson11 May 6, 2026 10:27

bemerybmw requested changes May 7, 2026

View reviewed changes

Conversation

KrishaDeshkool commented Apr 24, 2026

Uh oh!

KrishaDeshkool commented Apr 27, 2026

Uh oh!

Uh oh!

Uh oh!

KrishaDeshkool May 4, 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!

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

KrishaDeshkool May 4, 2026 •

edited

Loading