Conversation
|
Documentation preview for this pull request is available at: |
Add score::common::visitor::EnumTraits<T> trait template plus a new payload_tags::enum_le tag and enum_serialized_descriptor. visit_as SFINAE-dispatches to the enum-aware descriptor when EnumTraits<T>::kHasNames is true, otherwise falls back to the previous integer-only behavior. The wire format (byte layout) is unchanged. This lets downstream code generators (e.g. FIBEX generation for DLT/ CANape tooling) recover enumerator names for proto-derived enums by specializing EnumTraits<T>, without any change for enums that don't register a trait. Also fixes signed-enum detection in the new visit_as overloads: std::is_signed<T> is only meaningful for arithmetic types and is always false for enum types (enums are not arithmetic types per the standard), even when their underlying type is signed. Added details::is_signed_enum<T>, which inspects std::underlying_type_t<T> and is only instantiated for enum types, and use it in place of std::is_signed<T> for the enum dispatch. Added unit test coverage in test_serializer_visitor.cpp: - Verifies EnumTraits<T>::kHasNames defaults to false for enums without a registered trait, and true once specialized. - Verifies the serialized wire size is unchanged for a named enum. - Verifies visit_as dispatches to the enum_le payload tag and exposes the correct enum_type and wire_tag (signed_le for enums with a signed underlying type). - Verifies the registered enumerator name<->value table (kEnumerators) is actually retrievable through the trait. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
e8515eb to
02a50e1
Compare
|
@rmaddikery, @hoppe-and-dreams, please check this PR |
hoppe-and-dreams
left a comment
There was a problem hiding this comment.
Thanks for the contribution. Looks well in general. I added some minor findings to please address.
|
|
||
| } // namespace payload_tags | ||
|
|
||
| // One value <-> name mapping of an enumerator, as declared in the originating proto `enum`. |
There was a problem hiding this comment.
This comment is misleading. This library is independent from proto. This might be you use case, but it is generally not correct to use this as the only use case of this library.
| // | ||
| // The primary (unspecialized) template intentionally carries no names: any C++ enum works with | ||
| // the existing signed/unsigned integral serialization path unless a specialization of this trait | ||
| // is provided for it. Code generators are expected to emit a specialization for every proto |
There was a problem hiding this comment.
Again, the whole proto description is not relevant for this library in S-CORE.
The general comment, about the difference between how "specialized" enums vs regular enums are handle suffices.
| // that downstream consumers (e.g. the FIBEX generator's visitor) can look up EnumTraits<T> and | ||
| // recover the enumerator name<->value table. This does not change the serialized byte layout. | ||
| template <typename Tag, typename T> | ||
| // coverity[autosar_cpp14_a11_0_2_violation] |
There was a problem hiding this comment.
No coverity in S-CORE repository
| typename T, | ||
| std::enable_if_t<(std::is_enum<T>::value) && (details::is_signed_enum<T>::value) && (EnumTraits<T>::kHasNames), | ||
| std::int32_t> = 0> | ||
| /// \brief Dispatches a signed enum with a registered EnumTraits<T> specialization to the |
There was a problem hiding this comment.
Usually the comments go before the template< ... > line
| // coverity[autosar_cpp14_m3_2_3_violation : FALSE] | ||
| // coverity[autosar_cpp14_a2_10_4_violation : FALSE] |
| /// unsigned integer representation as memcpy_serialized_descriptor, but additionally | ||
| /// exposing T so that EnumTraits<T> can be looked up by downstream consumers. | ||
| // This is false positive, Overload signatures are different. | ||
| // coverity[autosar_cpp14_m3_2_3_violation : FALSE] |
There was a problem hiding this comment.
More coverity here
There was a problem hiding this comment.
Its fine for documentation of intent w.r.t to suppression until a new tool is introduced where the suppression could be removed OR carried over.
| RecordProperty("ParentRequirement", "SCR-1633893"); | ||
| RecordProperty("ASIL", "B"); | ||
| RecordProperty("Description", | ||
| "Check that registering EnumTraits<T> for an enum type only adds compile-time " | ||
| "enumerator name metadata and does not alter its serialized wire representation."); | ||
| RecordProperty("TestingTechnique", "Requirements-based test"); | ||
| RecordProperty("DerivationTechnique", "requirements-analysis"); // requirements |
There was a problem hiding this comment.
None of this makes sense in S-CORE, does it @4og ?
There was a problem hiding this comment.
It actually does, but the values are slightly different in S-CORE. The best is to see them in this markdown: https://github.com/eclipse-score/baselibs/blob/main/.agents/skills/requirements-management-skill/test-to-requirement-linking.md
We currently have an epic to clean all of this up across the codebase: #522
For this PR, I would just make the Description, TestingTechnique, DerivationTechnique aligned with the S-CORE guidelines (basically, only the TestingTechnique needs to be turned to ("TestType", "requirements-based"). For the rest I don't care for now.
Summary
Adds
score::common::visitor::EnumTraits<T>— a compile-time trait template that lets an enum type register its enumerator name<->value table — plus a newpayload_tags::enum_letag andenum_serialized_descriptor.visit_asnow SFINAE-dispatches to the enum-aware descriptor whenEnumTraits<T>::kHasNamesis true, and falls back to the previous integer-onlymemcpy_serialized_descriptorpath otherwise (defaultkHasNames = false, so any enum without a registered trait is unaffected).The wire format (serialized byte layout) is unchanged —
enum_serialized_descriptoruses the samememcpy_serialized<sizeof(T)>payload type as before, only exposing extra compile-time metadata (wire_tag,enum_type) for downstream consumers.This lets downstream code generators (e.g. FIBEX generation for DLT/CANape tooling) recover proto enumerator names for enum-typed fields, which are otherwise lost once the enum is folded into a plain integral wire type.
Also fixes a latent signed/unsigned dispatch bug introduced by the new enum overloads:
std::is_signed<T>is only meaningful for arithmetic types and is alwaysfalsefor enum types (enums are not arithmetic types per the standard), even when the enum's underlying type is signed. Addeddetails::is_signed_enum<T>, which inspectsstd::underlying_type_t<T>and is only instantiated for enum types, and use it instead ofstd::is_signed<T>for the enum dispatch — otherwise every enum would incorrectly getwire_tag = unsigned_le.Testing
Added
serializer_visitor.named_enum_wire_format_unchangedunit test intest_serializer_visitor.cpp:EnumTraits<test::EnumWithNames>and verifieskHasNamesdefaults tofalsefor enums without a trait (e.g. the pre-existingtest::E).check_serialized<test::EnumWithNames>() == sizeof(test::EnumWithNames)(wire format unchanged).visit_asdescriptor for a named enum usespayload_tags::enum_le, exposes the correctenum_type, and dispatches towire_tag == payload_tags::signed_lefor an enum with a signed underlying type (regression check for the signed/unsigned dispatch fix).kEnumerators) is actually retrievable through the trait.bazel test //score/static_reflection_with_serialization/serialization:unit_tests— all pass (serializer_ut,size_visitor_ut,visitor_type_traits_ut).