Add field initializing constructor, fix explicit, fix deserialization… (

#320) … 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.
OpenCyphal · Oct 18, 2023 · c7d4ced · c7d4ced
1 parent f78e3f7
commit c7d4ced
Show file tree

Hide file tree

Showing 10 changed files with 218 additions and 65 deletions.
diff --git a/src/nunavut/lang/_config.py b/src/nunavut/lang/_config.py
@@ -33,9 +33,21 @@ def parse_string(s: str) -> typing.Optional[typing.Any]:  # annoying mypy cheat
 
 
 class SpecialMethod(Enum):
-    DefaultConstructorWithOptionalAllocator = auto()
+    """
+    Enum used in the Jinja templates to differentiate different kinds of constructrors
+    """
+
+    AllocatorConstructor = auto()
+    """ Constructor that takes an allocator as its single, required argument """
+
+    InitializingConstructorWithAllocator = auto()
+    """ Constructor that takes an initializing value for each field followed by the allocator argument """
+
     CopyConstructorWithAllocator = auto()
+    """ Copy constructor that also takes an allocator argument """
+
     MoveConstructorWithAllocator = auto()
+    """ Move constructor that also takes an allocator argument """
 
 
 class LanguageConfig:

diff --git 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,25 +985,45 @@ 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:
     """
     Emit an initialization expression for a C++ special method.
     """
 
     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
@@ -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,8 @@ struct {{composite_type|short_reference_name}} final
         {{ field | id }}{{ field | value_initializer(SpecialMethod.CopyConstructorWithAllocator) }}{%if not loop.last %},{%endif %}
     {%- endfor %}
     {
+        (void)rhs;       // avoid unused param warning
+        (void)allocator; // avoid unused param warning
     }
 
     // Move constructor
@@ -118,13 +146,18 @@ struct {{composite_type|short_reference_name}} final
         {{ field | id }}{{ field | value_initializer(SpecialMethod.MoveConstructorWithAllocator) }}{%if not loop.last %},{%endif %}
     {%- endfor %}
     {
+        (void)rhs;       // avoid unused param warning
+        (void)allocator; // avoid unused param warning
     }
 
     // Copy assignment
     {{composite_type|short_reference_name}}& operator=(const {{composite_type|short_reference_name}}&) = default;
 
     // 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
@@ -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
diff --git 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<std::byte, 200> buffer{};
+    std::array<std::byte, 500> buffer{};
     std::pmr::monotonic_buffer_resource mbr{buffer.data(), buffer.size(), std::pmr::null_memory_resource()};
-    std::pmr::polymorphic_allocator<mymsgs::Outer_1_0> pa{&mbr};
+    std::pmr::polymorphic_allocator<mymsgs::OuterMore_1_0> 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<unsigned char, mymsgs::Outer_1_0::_traits_::SerializationBufferSizeBytes> roundtrip_buffer{};
+    std::array<unsigned char, mymsgs::OuterMore_1_0::_traits_::SerializationBufferSizeBytes> 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<const unsigned char*>(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);
+
 }