mw/com/impl: Internally use memory::DataTypeSizeInfo by crimson11 · Pull Request #813 · eclipse-score/communication

crimson11 · 2026-07-30T13:28:53Z

The type DataTypeMetaInfo, which we use on our
public interface is too weak! It doesn't assure
invariants on size/alignment, which C++ demands:
size has to be always a power of two and be an
integer multiple of tzhe alignment.

This commit switches to internal use of memory::DataTypeSizeInfo which forces this. On the public interface we keep the weak DataTypeMetaInfo for now to avoid a breaking change! In the public layer we transform DataTypeMetaInfo to DataTypeSizeInfo. In case this isn't possible because DataTypeMetaInfo is invalid, we return an error in the public API.

LittleHuba

I'd like to discuss different approaches to this topic.
This is not a hard no, I just want to brainstorm for ideas.

crimson11 · 2026-07-30T14:50:10Z

I'd like to discuss different approaches to this topic. This is not a hard no, I just want to brainstorm for ideas.

Sure! I'm curious for your proposals! Low level I want to "extend" a type used in the public API ... and I have no clue how to do this minimal invasive - opposed to creating a NEW type and deprecate all APIs with the old type and introduce new/overloads with the new Type! In this case that would even cascade as DataTypeMetaInfo is enclosed in other public types ... so this would be a crazy escalation!

bemerybmw · 2026-08-03T06:30:12Z

+
+TEST(MakeDataTypeSizeInfoTest, ConvertsValidMetaInfo)
+{
+    RecordProperty("Description",


We shouldn't have these properties if we're not linking a requirement imo. They can easily get out of sync with the code and the tests should be clear from the name and given/when/then documentation.

bemerybmw · 2026-08-03T06:31:37Z

+namespace
+{
+
+TEST(MakeDataTypeSizeInfoTest, ConvertsValidMetaInfo)


ConvertsSizeMultipleOfAlignment

bemerybmw · 2026-08-03T06:33:14Z


+TEST_F(GenericSkeletonTest, CreateWithInvalidDataTypeMetaInfoAlignmentFails)
+{
+    RecordProperty("Description",


Same thing, remove recordproperty

same for the other test

bemerybmw · 2026-08-03T06:45:39Z

+    GenericSkeletonServiceElementInfo params;
+    params.events = event_storage;
+
+    // The event factory must never be called for invalid meta-info


Suggested change

// The event factory must never be called for invalid meta-info

// Expecting that the event factory is never be called for invalid meta-info

same for the other test

bemerybmw · 2026-08-03T06:55:51Z

+        {
+            score::mw::log::LogError("GenericSkeleton")
+                << "Invalid data type meta-info provided for event: " << info.name;
+            return MakeUnexpected(ComErrc::kInvalidConfiguration);


Should this really be an invalidConfiguration error? I associate kInvalidConfiguration with the mw_com_config. But this is an error coming from C++. And do we want to return an error here or simply terminate? Because they can only be providing a data_type_meta_info which is invalid if they are doing something weird and not simply creating with sizeof / alignof.

kInvalidConfiguration was the only available/existing error-code, which looked not completely "off" :)
... and the DataTypeMetaInfo can be imho sen as the configuration of a GenericSkeleton ...
I could introduce a new error code, if you really want:
kInvalidDataTypeSize ?

In this case, I would "prefer" not to terminate ... (it is always a philosophical question): This is not a somewhat internal broken state, where we can't recover from. It is a clear call/argument violation. And if you look at how GenericSkeleton is being used, it makes sense NOT to terminate! The typical users (like these data-collectors) get some info from the backend! They are not doing sizeof/alignof! If they could do sizeof/alignof, thzen there would be no need for a GenericSkeleton ... so imho terminate is too harsh here!

bemerybmw · 2026-08-03T07:44:30Z

Why is this file here and not in plumbing?

good question! Will fix in a separate commit.

bemerybmw · 2026-08-03T07:45:44Z

 #ifndef SCORE_MW_COM_IMPL_PLUMBING_SKELETON_SERVICE_ELEMENT_BINDING_FACTORY_IMPL_H
 #define SCORE_MW_COM_IMPL_PLUMBING_SKELETON_SERVICE_ELEMENT_BINDING_FACTORY_IMPL_H

+#include "score/memory/data_type_size_info.h"


The first block of includes are includes from the same package/ component, so this should be in the second block i.e. with logging

bemerybmw · 2026-08-03T07:46:55Z

 #define SCORE_MW_COM_IMPL_PLUMBING_GENERIC_SKELETON_EVENT_BINDING_FACTORY_MOCK_H

-#include "score/mw/com/impl/data_type_meta_info.h"
+#include "score/memory/data_type_size_info.h"


The first block of includes are includes from the same package/ component, so this should be in a separate second block

Suggested change

#include "score/memory/data_type_size_info.h"

#include "score/mw/com/impl/i_generic_skeleton_event_binding_factory.h"

#include "score/memory/data_type_size_info.h"

bemerybmw · 2026-08-03T07:47:47Z


-#include "score/mw/com/impl/data_type_meta_info.h"
+#include "score/memory/data_type_size_info.h"
 #include "score/mw/com/impl/i_generic_skeleton_event_binding_factory.h"


Suggested change

#include "score/mw/com/impl/i_generic_skeleton_event_binding_factory.h"

#include "score/mw/com/impl/i_generic_skeleton_event_binding_factory.h"

#include "score/memory/data_type_size_info.h"

bemerybmw · 2026-08-03T07:49:57Z

 #ifndef SCORE_MW_COM_IMPL_BINDINGS_LOLA_GENERIC_SKELETON_EVENT_H_
 #define SCORE_MW_COM_IMPL_BINDINGS_LOLA_GENERIC_SKELETON_EVENT_H_

+#include "score/memory/data_type_size_info.h"


Suggested change

#include "score/memory/data_type_size_info.h"

#include "score/mw/com/impl/bindings/lola/element_fq_id.h"

#include "score/mw/com/impl/bindings/lola/event_data_storage.h"

#include "score/mw/com/impl/bindings/lola/skeleton_event_common.h"

#include "score/mw/com/impl/bindings/lola/skeleton_event_properties.h"

#include "score/mw/com/impl/generic_skeleton_event_binding.h"

#include "score/mw/com/impl/sample_allocatee_guard.h"

#include "score/memory/data_type_size_info.h"

bemerybmw · 2026-08-03T07:54:57Z

        "//score/mw/com/impl/test:dummy_instance_identifier_builder",
        "//score/mw/com/impl/test:runtime_mock_guard",
        "@googletest//:gtest",
+        "@score_baselibs//score/memory:data_type_size_info",


Should now be referencing target in the com repo?

nope. //score/memory hasn't been moved.

The type DataTypeMetaInfo, which we use on our public interface is too weak! It doesn't assure invariants on size/alignment, which C++ demands: size has to be always a power of two and be an integer multiple of tzhe alignment. This commit switches to internal use of memory::DataTypeSizeInfo which forces this. On the public interface we keep the weak DataTypeMetaInfo for now to avoid a breaking change! In the public layer we transform DataTypeMetaInfo to DataTypeSizeInfo. In case this isn't possible because DataTypeMetaInfo is invalid, we return an error in the public API.

IGenericSkeletonEventBindingFactory has been shifted to plumbing folder.

                                           const SkeletonEventProperties& event_properties,
                                           const ElementFqId& element_fq_id,
-                                           const DataTypeMetaInfo& size_info,
+                                           const memory::DataTypeSizeInfo& size_info,


crimson11 requested review from LittleHuba, bemerybmw, castler, hoe-jo and limdor as code owners July 30, 2026 13:28

github-project-automation Bot added this to COM - Communication FT Jul 30, 2026

github-project-automation Bot moved this to Backlog in COM - Communication FT Jul 30, 2026

crimson11 force-pushed the mf_use_datatype_size_info branch from 098d64a to a84541a Compare July 30, 2026 14:10

LittleHuba requested changes Jul 30, 2026

View reviewed changes

crimson11 force-pushed the mf_use_datatype_size_info branch 3 times, most recently from 7802866 to a0334af Compare July 31, 2026 12:54

bemerybmw requested changes Aug 3, 2026

View reviewed changes

bemerybmw reviewed Aug 3, 2026

View reviewed changes

crimson11 force-pushed the mf_use_datatype_size_info branch from a0334af to ee884f0 Compare August 3, 2026 12:18

github-advanced-security AI found potential problems Aug 3, 2026

View reviewed changes

Comment thread score/mw/com/impl/bindings/lola/generic_skeleton_event.cpp Fixed

crimson11 force-pushed the mf_use_datatype_size_info branch from ee884f0 to 25fead2 Compare August 3, 2026 15:00

crimson11 added 2 commits August 3, 2026 17:05

mw/com/impl: shifted IGenericSkeletonEventBindingFactory

6e9aa5c

IGenericSkeletonEventBindingFactory has been shifted to plumbing folder.

crimson11 force-pushed the mf_use_datatype_size_info branch from 25fead2 to 6e9aa5c Compare August 3, 2026 15:05

crimson11 requested review from LittleHuba and bemerybmw August 3, 2026 15:06

github-advanced-security AI found potential problems Aug 3, 2026

View reviewed changes

Comment thread score/mw/com/impl/bindings/lola/generic_skeleton_event.cpp

const SkeletonEventProperties& event_properties,

const ElementFqId& element_fq_id,

const DataTypeMetaInfo& size_info,

const memory::DataTypeSizeInfo& size_info,

	// The event factory must never be called for invalid meta-info
	// Expecting that the event factory is never be called for invalid meta-info

-#include "score/memory/data_type_size_info.h"
+#include "score/mw/com/impl/bindings/lola/element_fq_id.h"
+#include "score/mw/com/impl/bindings/lola/event_data_storage.h"
+#include "score/mw/com/impl/bindings/lola/skeleton_event_common.h"
+#include "score/mw/com/impl/bindings/lola/skeleton_event_properties.h"
+#include "score/mw/com/impl/generic_skeleton_event_binding.h"
+#include "score/mw/com/impl/sample_allocatee_guard.h"
+#include "score/memory/data_type_size_info.h"

Uh oh!

Conversation

crimson11 commented Jul 30, 2026

Uh oh!

LittleHuba left a comment

Choose a reason for hiding this comment

Uh oh!

crimson11 commented Jul 30, 2026

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!

crimson11 Aug 3, 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!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

crimson11 Aug 3, 2026 •

edited

Loading