From 93431d8b6302aca32bb1eb5a28f02e37a1d02378 Mon Sep 17 00:00:00 2001 From: Skeets Norquist Date: Wed, 4 Oct 2023 14:58:17 -0700 Subject: [PATCH] Add field initializing constructor, fix explicit, fix deserialization bug When you use "cetl++14-17" or "c++17-pmr" we code-gen message classes with constructors that take an allocator. Since the class has a user-defined constructor this means it can no longer use aggregate initialization (https://en.cppreference.com/w/cpp/language/aggregate_initialization) This is inconvenient so I'm adding a constructor with args for each field, in order. This also makes it possible to declare a const message instance. Some single arg constructors weren't marked explicit, thus becoming user-defined conversion functions, which introduces unexpected bugs. I've fixed all the constructors so any single-arg ones are declared explicit. Fixed a deserialization bug where the allocator was not getting passed to a temporary. --- src/nunavut/lang/_config.py | 3 +- src/nunavut/lang/cpp/__init__.py | 84 +++++++++++++++---- .../lang/cpp/templates/_composite_type.j2 | 43 ++++++++-- .../lang/cpp/templates/deserialization.j2 | 2 +- submodules/CETL | 2 +- .../cpp/suite/test_array_c++17-pmr.cpp | 62 +++++++++----- .../cpp/suite/test_array_cetl++14-17.cpp | 63 +++++++++----- .../mymsgs/InnerMore.1.0.dsdl | 3 + .../mymsgs/OuterMore.1.0.dsdl | 4 + .../nested_array_types/mymsgs/Simple.1.0.dsdl | 4 + 10 files changed, 205 insertions(+), 65 deletions(-) create mode 100644 verification/nunavut_test_types/nested_array_types/mymsgs/InnerMore.1.0.dsdl create mode 100644 verification/nunavut_test_types/nested_array_types/mymsgs/OuterMore.1.0.dsdl create mode 100644 verification/nunavut_test_types/nested_array_types/mymsgs/Simple.1.0.dsdl diff --git a/src/nunavut/lang/_config.py b/src/nunavut/lang/_config.py index 0853fb51..ad6291a5 100644 --- a/src/nunavut/lang/_config.py +++ b/src/nunavut/lang/_config.py @@ -33,7 +33,8 @@ def parse_string(s: str) -> typing.Optional[typing.Any]: # annoying mypy cheat class SpecialMethod(Enum): - DefaultConstructorWithOptionalAllocator = auto() + AllocatorConstructor = auto() + InitializingConstructorWithAllocator = auto() CopyConstructorWithAllocator = auto() MoveConstructorWithAllocator = auto() diff --git a/src/nunavut/lang/cpp/__init__.py b/src/nunavut/lang/cpp/__init__.py index 5c806ba7..547e6d0d 100644 --- a/src/nunavut/lang/cpp/__init__.py +++ b/src/nunavut/lang/cpp/__init__.py @@ -923,16 +923,41 @@ def filter_destructor_name(language: Language, instance: pydsdl.Any) -> str: return "~" + "<".join(declaration_parts) +@template_language_filter(__name__) +def filter_explicit_decorator(language: Language, instance: pydsdl.Any, special_method: SpecialMethod) -> str: + """ + Emit the constructor name, decorated with "explicit" if it has only one argument + """ + name: str = language.filter_short_reference_name(instance) + arg_count: int = len(instance.fields_except_padding) + ( + 0 if language.get_option("allocator_is_default_constructible") else 1 + ) + if special_method == SpecialMethod.InitializingConstructorWithAllocator and arg_count == 1: + return f"explicit {name}" + else: + return f"{name}" + + @template_language_filter(__name__) def filter_default_value_initializer(language: Language, instance: pydsdl.Any) -> str: """ - Emit a default initialization statement for the given instance if primitive or array. + Emit a default initialization expression for the given instance if primitive, array, + or composite. """ - if isinstance(instance, pydsdl.PrimitiveType) or isinstance(instance, pydsdl.ArrayType): + if ( + isinstance(instance, pydsdl.PrimitiveType) + or isinstance(instance, pydsdl.ArrayType) + or isinstance(instance, pydsdl.CompositeType) + ): return "{}" return "" +def needs_initializing_value(special_method: SpecialMethod) -> bool: + """Helper method used by filter_value_initializer()""" + return special_method == SpecialMethod.InitializingConstructorWithAllocator or needs_rhs(special_method) + + def needs_rhs(special_method: SpecialMethod) -> bool: """Helper method used by filter_value_initializer()""" return special_method in ( @@ -950,7 +975,7 @@ def needs_allocator(instance: pydsdl.Any) -> bool: def needs_vla_init_args(instance: pydsdl.Any, special_method: SpecialMethod) -> bool: """Helper method used by filter_value_initializer()""" - return special_method == SpecialMethod.DefaultConstructorWithOptionalAllocator and isinstance( + return special_method == SpecialMethod.AllocatorConstructor and isinstance( instance.data_type, pydsdl.VariableLengthArrayType ) @@ -960,6 +985,28 @@ def needs_move(special_method: SpecialMethod) -> bool: return special_method == SpecialMethod.MoveConstructorWithAllocator +def requires_initialization(instance: pydsdl.Any) -> bool: + """Helper method used by filter_value_initializer()""" + return ( + isinstance(instance.data_type, pydsdl.PrimitiveType) + or isinstance(instance.data_type, pydsdl.ArrayType) + or isinstance(instance.data_type, pydsdl.CompositeType) + ) + + +def assemble_initializer_expression( + wrap: str, rhs: str, leading_args: typing.List[str], trailing_args: typing.List[str] +) -> str: + """Helper method used by filter_value_initializer()""" + if wrap: + rhs = "{}({})".format(wrap, rhs) + args = [] + if rhs: + args.append(rhs) + args = leading_args + args + trailing_args + return "{" + ", ".join(args) + "}" + + @template_language_filter(__name__) def filter_value_initializer(language: Language, instance: pydsdl.Any, special_method: SpecialMethod) -> str: """ @@ -967,18 +1014,16 @@ def filter_value_initializer(language: Language, instance: pydsdl.Any, special_m """ value_initializer: str = "" - if ( - isinstance(instance.data_type, pydsdl.PrimitiveType) - or isinstance(instance.data_type, pydsdl.ArrayType) - or isinstance(instance.data_type, pydsdl.CompositeType) - ): + if requires_initialization(instance): wrap: str = "" rhs: str = "" leading_args: typing.List[str] = [] trailing_args: typing.List[str] = [] - if needs_rhs(special_method): - rhs = "rhs." + language.filter_id(instance) + if needs_initializing_value(special_method): + if needs_rhs(special_method): + rhs = "rhs." + rhs += language.filter_id(instance) if needs_vla_init_args(instance, special_method): constructor_args = language.get_option("variable_array_type_constructor_args") @@ -994,17 +1039,22 @@ def filter_value_initializer(language: Language, instance: pydsdl.Any, special_m if needs_move(special_method): wrap = "std::move" - if wrap: - rhs = "{}({})".format(wrap, rhs) - args = [] - if rhs: - args.append(rhs) - args = leading_args + args + trailing_args - value_initializer = "{" + ", ".join(args) + "}" + value_initializer = assemble_initializer_expression(wrap, rhs, leading_args, trailing_args) return value_initializer +@template_language_filter(__name__) +def filter_default_construction(language: Language, instance: pydsdl.Any, reference: str) -> str: + if ( + isinstance(instance, pydsdl.CompositeType) + and language.get_option("ctor_convention") != ConstructorConvention.Default.value + ): + return f"{reference}.get_allocator()" + else: + return "" + + @template_language_filter(__name__) def filter_declaration(language: Language, instance: pydsdl.Any) -> str: """ diff --git a/src/nunavut/lang/cpp/templates/_composite_type.j2 b/src/nunavut/lang/cpp/templates/_composite_type.j2 index cee7359d..34bb0785 100644 --- a/src/nunavut/lang/cpp/templates/_composite_type.j2 +++ b/src/nunavut/lang/cpp/templates/_composite_type.j2 @@ -84,17 +84,43 @@ struct {{composite_type|short_reference_name}} final {%- endfor %} }; {% if options.ctor_convention != ConstructorConvention.Default.value %} + {%- if options.allocator_is_default_constructible %} // Default constructor - {{composite_type|short_reference_name}}(const _traits_::allocator_type& allocator - {%- if options.allocator_is_default_constructible %} = _traits_::allocator_type(){% endif %}) + {{composite_type|short_reference_name}}() {%- if composite_type.fields_except_padding %} :{% endif %} {%- for field in composite_type.fields_except_padding %} - {{ field | id }}{{ field | value_initializer(SpecialMethod.DefaultConstructorWithOptionalAllocator) }}{%if not loop.last %},{%endif %} + {{ field | id }}{{ field.data_type | default_value_initializer }}{%if not loop.last %},{%endif %} {%- endfor %} - { } + { + } + {%- endif %} - // Destructor - ~{{composite_type|short_reference_name}}() = default; + // Allocator constructor + explicit {{composite_type|short_reference_name}}(const _traits_::allocator_type& allocator) + {%- if composite_type.fields_except_padding %} :{% endif %} + {%- for field in composite_type.fields_except_padding %} + {{ field | id }}{{ field | value_initializer(SpecialMethod.AllocatorConstructor) }}{%if not loop.last %},{%endif %} + {%- endfor %} + { + (void)allocator; // avoid unused param warning + } + + {% if composite_type.fields_except_padding %} + // Initializing constructor + {{ composite_type | explicit_decorator(SpecialMethod.InitializingConstructorWithAllocator)}}( + {%- for field in composite_type.fields_except_padding %} + const _traits_::TypeOf::{{ field | id }}& {{ field | id }}, + {%- endfor %} + const _traits_::allocator_type& allocator + {%- if options.allocator_is_default_constructible %} = _traits_::allocator_type(){% endif %}) + {%- if composite_type.fields_except_padding %} :{% endif %} + {%- for field in composite_type.fields_except_padding %} + {{ field | id }}{{ field | value_initializer(SpecialMethod.InitializingConstructorWithAllocator) }}{%if not loop.last %},{%endif %} + {%- endfor %} + { + (void)allocator; // avoid unused param warning + } + {%- endif %} // Copy constructor {{composite_type|short_reference_name}}(const {{composite_type|short_reference_name}}&) = default; @@ -106,6 +132,7 @@ struct {{composite_type|short_reference_name}} final {{ field | id }}{{ field | value_initializer(SpecialMethod.CopyConstructorWithAllocator) }}{%if not loop.last %},{%endif %} {%- endfor %} { + (void)allocator; // avoid unused param warning } // Move constructor @@ -118,6 +145,7 @@ struct {{composite_type|short_reference_name}} final {{ field | id }}{{ field | value_initializer(SpecialMethod.MoveConstructorWithAllocator) }}{%if not loop.last %},{%endif %} {%- endfor %} { + (void)allocator; // avoid unused param warning } // Copy assignment @@ -125,6 +153,9 @@ struct {{composite_type|short_reference_name}} final // Move assignment {{composite_type|short_reference_name}}& operator=({{composite_type|short_reference_name}}&&) = default; + + // Destructor + ~{{composite_type|short_reference_name}}() = default; {%- endif %} {%- for constant in composite_type.constants %} diff --git a/src/nunavut/lang/cpp/templates/deserialization.j2 b/src/nunavut/lang/cpp/templates/deserialization.j2 index 397a8332..588f63f4 100644 --- a/src/nunavut/lang/cpp/templates/deserialization.j2 +++ b/src/nunavut/lang/cpp/templates/deserialization.j2 @@ -204,7 +204,7 @@ {% set tmp_element = 'tmp'|to_template_unique_name %} for ({{ typename_unsigned_length }} {{ ref_index }} = 0U; {{ ref_index }} < {{ ref_size }}; ++{{ ref_index }}) { - {{ t.element_type | declaration }} {{ tmp_element }} = {{ t.element_type | declaration }}(); + {{ t.element_type | declaration }} {{ tmp_element }} = {{ t.element_type | declaration }}({{ t.element_type | default_construction(reference) }}); {{ _deserialize_any(t.element_type, tmp_element, element_offset) |trim|indent diff --git a/submodules/CETL b/submodules/CETL index 64209d50..d2f0af34 160000 --- a/submodules/CETL +++ b/submodules/CETL @@ -1 +1 @@ -Subproject commit 64209d50290b5060095554ef544a8ea7c3245e75 +Subproject commit d2f0af34f7d5487b3004ba9b95fb9d8cd42f0174 diff --git a/verification/cpp/suite/test_array_c++17-pmr.cpp b/verification/cpp/suite/test_array_c++17-pmr.cpp index d8596f05..07b58569 100644 --- a/verification/cpp/suite/test_array_c++17-pmr.cpp +++ b/verification/cpp/suite/test_array_c++17-pmr.cpp @@ -9,7 +9,9 @@ #include "gmock/gmock.h" #include "mymsgs/Inner_1_0.hpp" +#include "mymsgs/InnerMore_1_0.hpp" #include "mymsgs/Outer_1_0.hpp" +#include "mymsgs/OuterMore_1_0.hpp" /** @@ -69,38 +71,60 @@ TEST(StdVectorPmrTests, TestOuter) { * Serialization roundtrip using std::pmr::polymorphic_allocator */ TEST(StdVectorTests, SerializationRoundtrip) { - std::array buffer{}; + std::array buffer{}; std::pmr::monotonic_buffer_resource mbr{buffer.data(), buffer.size(), std::pmr::null_memory_resource()}; - std::pmr::polymorphic_allocator pa{&mbr}; + std::pmr::polymorphic_allocator pa{&mbr}; // Fill the data: inner first, then outer - mymsgs::Outer_1_0 outer1{pa}; - outer1.inner.inner_items.reserve(5); - for (std::uint32_t i = 0; i < 5; i++) { - outer1.inner.inner_items.push_back(i); - } - outer1.outer_items.reserve(8); - for (float i = 0; i < 8; i++) { - outer1.outer_items.push_back(i + 5.0f); - } + const mymsgs::OuterMore_1_0 outer1( + {{1, 2, 3, 4}, pa}, // float32[<=8] outer_items + { // InnerMore.1.0[<=2] inners + { + { // InnerMore_1_0 + {{55, 66, 77}, pa}, // uint32[<=5] inner_items + false, // bool inner_primitive + pa + }, + { // InnerMore_1_0 + {{88, 99}, pa}, // uint32[<=5] inner_items + true, // bool inner_primitive + pa + } + }, + pa + }, + 777777, // int64 outer_primitive + pa + ); // Serialize it - std::array roundtrip_buffer{}; + std::array roundtrip_buffer{}; nunavut::support::bitspan ser_buffer(roundtrip_buffer); const auto ser_result = serialize(outer1, ser_buffer); ASSERT_TRUE(ser_result); // Deserialize it - mymsgs::Outer_1_0 outer2{pa}; + mymsgs::OuterMore_1_0 outer2{pa}; nunavut::support::const_bitspan des_buffer(static_cast(roundtrip_buffer.data()), roundtrip_buffer.size()); const auto des_result = deserialize(outer2, des_buffer); ASSERT_TRUE(des_result); // Verify that the messages match - for (std::uint32_t i = 0; i < 5; i++) { - ASSERT_EQ(outer1.inner.inner_items[i], outer2.inner.inner_items[i]); - } - for (std::uint32_t i = 0; i < 8; i++) { - ASSERT_EQ(outer1.outer_items[i], outer2.outer_items[i]); - } + ASSERT_EQ(outer2.outer_items.size(), 4); + ASSERT_EQ(outer2.outer_items[0], 1); + ASSERT_EQ(outer2.outer_items[1], 2); + ASSERT_EQ(outer2.outer_items[2], 3); + ASSERT_EQ(outer2.outer_items[3], 4); + ASSERT_EQ(outer2.inners.size(), 2); + ASSERT_EQ(outer2.inners[0].inner_items.size(), 3); + ASSERT_EQ(outer2.inners[0].inner_items[0], 55); + ASSERT_EQ(outer2.inners[0].inner_items[1], 66); + ASSERT_EQ(outer2.inners[0].inner_items[2], 77); + ASSERT_EQ(outer2.inners[0].inner_primitive, false); + ASSERT_EQ(outer2.inners[1].inner_items.size(), 2); + ASSERT_EQ(outer2.inners[1].inner_items[0], 88); + ASSERT_EQ(outer2.inners[1].inner_items[1], 99); + ASSERT_EQ(outer2.inners[1].inner_primitive, true); + ASSERT_EQ(outer2.outer_primitive, 777777); + } diff --git a/verification/cpp/suite/test_array_cetl++14-17.cpp b/verification/cpp/suite/test_array_cetl++14-17.cpp index 5eff6d0d..694785f2 100644 --- a/verification/cpp/suite/test_array_cetl++14-17.cpp +++ b/verification/cpp/suite/test_array_cetl++14-17.cpp @@ -8,7 +8,9 @@ #include "cetl/pf17/byte.hpp" #include "cetl/pf17/sys/memory_resource.hpp" #include "mymsgs/Inner_1_0.hpp" +#include "mymsgs/InnerMore_1_0.hpp" #include "mymsgs/Outer_1_0.hpp" +#include "mymsgs/OuterMore_1_0.hpp" /** @@ -68,38 +70,59 @@ TEST(CetlVlaPmrTests, TestOuter) { * Serialization roundtrip using cetl::pf17::pmr::polymorphic_allocator */ TEST(CetlVlaPmrTests, SerializationRoundtrip) { - std::array buffer{}; + std::array buffer{}; cetl::pf17::pmr::monotonic_buffer_resource mbr{buffer.data(), buffer.size(), cetl::pf17::pmr::null_memory_resource()}; - cetl::pf17::pmr::polymorphic_allocator pa{&mbr}; + cetl::pf17::pmr::polymorphic_allocator pa{&mbr}; - // Fill the data: inner first, then outer - mymsgs::Outer_1_0 outer1{pa}; - outer1.inner.inner_items.reserve(5); - for (std::uint32_t i = 0; i < 5; i++) { - outer1.inner.inner_items.push_back(i); - } - outer1.outer_items.reserve(8); - for (float i = 0; i < 8; i++) { - outer1.outer_items.push_back(i + 5.0f); - } + // Fill the data + const mymsgs::OuterMore_1_0 outer1( + {{1, 2, 3, 4}, pa}, // float32[<=8] outer_items + { // InnerMore.1.0[<=2] inners + { + { // InnerMore_1_0 + {{55, 66, 77}, pa}, // uint32[<=5] inner_items + false, // bool inner_primitive + pa + }, + { // InnerMore_1_0 + {{88, 99}, pa}, // uint32[<=5] inner_items + true, // bool inner_primitive + pa + } + }, + pa + }, + 777777, // int64 outer_primitive + pa + ); // Serialize it - std::array roundtrip_buffer{}; + std::array roundtrip_buffer{}; nunavut::support::bitspan ser_buffer(roundtrip_buffer); const auto ser_result = serialize(outer1, ser_buffer); ASSERT_TRUE(ser_result); // Deserialize it - mymsgs::Outer_1_0 outer2{pa}; + mymsgs::OuterMore_1_0 outer2{pa}; nunavut::support::const_bitspan des_buffer(static_cast(roundtrip_buffer.data()), roundtrip_buffer.size()); const auto des_result = deserialize(outer2, des_buffer); ASSERT_TRUE(des_result); // Verify that the messages match - for (std::uint32_t i = 0; i < 5; i++) { - ASSERT_EQ(outer1.inner.inner_items[i], outer2.inner.inner_items[i]); - } - for (std::uint32_t i = 0; i < 8; i++) { - ASSERT_EQ(outer1.outer_items[i], outer2.outer_items[i]); - } + ASSERT_EQ(outer2.outer_items.size(), 4); + ASSERT_EQ(outer2.outer_items[0], 1); + ASSERT_EQ(outer2.outer_items[1], 2); + ASSERT_EQ(outer2.outer_items[2], 3); + ASSERT_EQ(outer2.outer_items[3], 4); + ASSERT_EQ(outer2.inners.size(), 2); + ASSERT_EQ(outer2.inners[0].inner_items.size(), 3); + ASSERT_EQ(outer2.inners[0].inner_items[0], 55); + ASSERT_EQ(outer2.inners[0].inner_items[1], 66); + ASSERT_EQ(outer2.inners[0].inner_items[2], 77); + ASSERT_EQ(outer2.inners[0].inner_primitive, false); + ASSERT_EQ(outer2.inners[1].inner_items.size(), 2); + ASSERT_EQ(outer2.inners[1].inner_items[0], 88); + ASSERT_EQ(outer2.inners[1].inner_items[1], 99); + ASSERT_EQ(outer2.inners[1].inner_primitive, true); + ASSERT_EQ(outer2.outer_primitive, 777777); } diff --git a/verification/nunavut_test_types/nested_array_types/mymsgs/InnerMore.1.0.dsdl b/verification/nunavut_test_types/nested_array_types/mymsgs/InnerMore.1.0.dsdl new file mode 100644 index 00000000..58e72dcc --- /dev/null +++ b/verification/nunavut_test_types/nested_array_types/mymsgs/InnerMore.1.0.dsdl @@ -0,0 +1,3 @@ +uint32[<=5] inner_items +bool inner_primitive +@sealed diff --git a/verification/nunavut_test_types/nested_array_types/mymsgs/OuterMore.1.0.dsdl b/verification/nunavut_test_types/nested_array_types/mymsgs/OuterMore.1.0.dsdl new file mode 100644 index 00000000..9536dc1d --- /dev/null +++ b/verification/nunavut_test_types/nested_array_types/mymsgs/OuterMore.1.0.dsdl @@ -0,0 +1,4 @@ +float32[<=8] outer_items +InnerMore.1.0[<=2] inners +int64 outer_primitive +@sealed diff --git a/verification/nunavut_test_types/nested_array_types/mymsgs/Simple.1.0.dsdl b/verification/nunavut_test_types/nested_array_types/mymsgs/Simple.1.0.dsdl new file mode 100644 index 00000000..a18daa8b --- /dev/null +++ b/verification/nunavut_test_types/nested_array_types/mymsgs/Simple.1.0.dsdl @@ -0,0 +1,4 @@ +int32 a +float16 b +bool c +@sealed