Fix issues: allocator passing, verification test C++ flavors, union c…

…ode-gen (OpenCyphal#323) - Changed the verification tests CMakeLists.txt so it uses a matrix of test files to language flavors. This allows turning language flavors on or off for each test. - Updated the docker image tag used by the verification tests. - Fixed the c++17-pmr flavor to use "memory_resource" as the allocator include since that's where std::pmr::polymorphic_allocator is. - Exclude unions from all code-gen using allocator since there is not a way to pass it through to the elements of std::variant in a way that makes sense.
edu-rossrobotics · Nov 28, 2023 · 256fb06 · 256fb06
1 parent 7ecfd68
commit 256fb06
Show file tree

Hide file tree

Showing 9 changed files with 132 additions and 48 deletions.
diff --git a/CONTRIBUTING.rst b/CONTRIBUTING.rst
@@ -80,8 +80,8 @@ skip the docker invocations and use ``tox -s``.
 
 To run the language verification build you'll need to use a different docker container::
 
-    docker pull ghcr.io/opencyphal/toolshed:ts22.4.1
-    docker run --rm -it -v $PWD:/workspace ghcr.io/opencyphal/toolshed:ts22.4.1
+    docker pull ghcr.io/opencyphal/toolshed:ts22.4.3
+    docker run --rm -it -v $PWD:/workspace ghcr.io/opencyphal/toolshed:ts22.4.3
     cd /workspace
     ./.github/verify.py -l c
     ./.github/verify.py -l cpp

diff --git a/src/nunavut/_version.py b/src/nunavut/_version.py
@@ -8,7 +8,7 @@
 .. autodata:: __version__
 """
 
-__version__ = "2.3.1"
+__version__ = "2.3.2.dev0"
 __license__ = "MIT"
 __author__ = "OpenCyphal"
 __copyright__ = "Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved. Copyright (c) 2023 OpenCyphal."

diff --git a/src/nunavut/lang/cpp/templates/_composite_type.j2 b/src/nunavut/lang/cpp/templates/_composite_type.j2
@@ -33,6 +33,10 @@ struct {% if composite_type.deprecated -%}
 {%- endif -%}
 {{composite_type|short_reference_name}} final
 {
+{%- if options.ctor_convention != ConstructorConvention.Default.value %}
+    using allocator_type = {{ options.allocator_type }}<void>;
+{%- endif %}
+
     struct _traits_  // The name is surrounded with underscores to avoid collisions with DSDL attributes.
     {
         _traits_() = delete;
@@ -68,9 +72,6 @@ struct {% if composite_type.deprecated -%}
         static_assert({# -#}
             ExtentBytes < (std::numeric_limits<{{ typename_unsigned_bit_length }}>::max() / 8U), {# -#}
             "This message is too large to be handled by the selected types");
-{% if options.ctor_convention != ConstructorConvention.Default.value %}
-        using allocator_type = {{ options.allocator_type }}<decltype(nullptr)>;
-{% endif %}
 {%- for field in composite_type.fields_except_padding %}
     {%- if loop.first %}
         struct TypeOf
@@ -86,6 +87,9 @@ struct {% if composite_type.deprecated -%}
 {% if options.ctor_convention != ConstructorConvention.Default.value %}
     {%- if options.allocator_is_default_constructible %}
     // Default constructor
+    {%- if composite_type.inner_type is UnionType %}
+    {{composite_type|short_reference_name}}() = default;
+    {%- else %}
     {{composite_type|short_reference_name}}()
     {%- if composite_type.fields_except_padding %} :{% endif %}
     {%- for field in composite_type.fields_except_padding %}
@@ -94,25 +98,31 @@ struct {% if composite_type.deprecated -%}
     {
     }
     {%- endif %}
+    {%- endif %}
 
     // Allocator constructor
-    explicit {{composite_type|short_reference_name}}(const _traits_::allocator_type& allocator)
+    explicit {{composite_type|short_reference_name}}(const allocator_type& allocator)
     {%- if composite_type.fields_except_padding %} :{% endif %}
+    {%- if composite_type.inner_type is UnionType %}
+        union_value{} // can't make use of the allocator with a union
+    {%- else %}
     {%- for field in composite_type.fields_except_padding %}
         {{ field | id }}{{ field | value_initializer(SpecialMethod.AllocatorConstructor) }}{%if not loop.last %},{%endif %}
     {%- endfor %}
+    {%- endif %}
     {
         (void)allocator; // avoid unused param warning
     }
 
+    {%- if composite_type.inner_type is not UnionType %}
     {% 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 %})
+        const allocator_type& allocator
+    {%- if options.allocator_is_default_constructible %} = 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 %}
@@ -121,16 +131,21 @@ struct {% if composite_type.deprecated -%}
         (void)allocator; // avoid unused param warning
     }
     {%- endif %}
+    {%- endif %}
 
     // Copy constructor
     {{composite_type|short_reference_name}}(const {{composite_type|short_reference_name}}&) = default;
 
     // Copy constructor with allocator
-    {{composite_type|short_reference_name}}(const {{composite_type|short_reference_name}}& rhs, const _traits_::allocator_type& allocator)
+    {{composite_type|short_reference_name}}(const {{composite_type|short_reference_name}}& rhs, const allocator_type& allocator)
     {%- if composite_type.fields_except_padding %} :{% endif %}
+    {%- if composite_type.inner_type is UnionType %}
+        union_value{rhs.union_value}  // can't make use of the allocator with a union
+    {%- else %}
     {%- for field in composite_type.fields_except_padding %}
         {{ field | id }}{{ field | value_initializer(SpecialMethod.CopyConstructorWithAllocator) }}{%if not loop.last %},{%endif %}
     {%- endfor %}
+    {% endif %}
     {
         (void)rhs;       // avoid unused param warning
         (void)allocator; // avoid unused param warning
@@ -140,11 +155,15 @@ struct {% if composite_type.deprecated -%}
     {{composite_type|short_reference_name}}({{composite_type|short_reference_name}}&&) = default;
 
     // Move constructor with allocator
-    {{composite_type|short_reference_name}}({{composite_type|short_reference_name}}&& rhs, const _traits_::allocator_type& allocator)
+    {{composite_type|short_reference_name}}({{composite_type|short_reference_name}}&& rhs, const allocator_type& allocator)
     {%- if composite_type.fields_except_padding %} :{% endif %}
+    {%- if composite_type.inner_type is UnionType %}
+        union_value{} // can't make use of the allocator with a union
+    {%- else %}
     {%- for field in composite_type.fields_except_padding %}
         {{ field | id }}{{ field | value_initializer(SpecialMethod.MoveConstructorWithAllocator) }}{%if not loop.last %},{%endif %}
     {%- endfor %}
+    {%- endif %}
     {
         (void)rhs;       // avoid unused param warning
         (void)allocator; // avoid unused param warning

diff --git a/src/nunavut/lang/properties.yaml b/src/nunavut/lang/properties.yaml
@@ -332,7 +332,7 @@ nunavut.lang.cpp:
         variable_array_type_include: "<vector>"
         variable_array_type_template: "std::vector<{TYPE}, {REBIND_ALLOCATOR}>"
         variable_array_type_constructor_args: ""
-        allocator_include: "<memory>"
+        allocator_include: "<memory_resource>"
         allocator_type: "std::pmr::polymorphic_allocator"
         allocator_is_default_constructible: true
         ctor_convention: "uses-trailing-allocator"

diff --git a/test/gentest_arrays/test_arrays.py b/test/gentest_arrays/test_arrays.py
@@ -82,7 +82,7 @@ def test_var_array_override_cpp(gen_paths):  # type: ignore
         gen_paths.find_outfile_in_namespace("radar.Phased", namespace),
         re.compile(r'^#include "scotec/alloc.hpp"$'),
         re.compile(r'^#include "scotec/array.hpp"$'),
-        re.compile(r".*\bconst _traits_::allocator_type& allocator = _traits_::allocator_type()"),
+        re.compile(r".*\bconst allocator_type& allocator = allocator_type()"),
         re.compile(r"\s*using *antennae_per_bank *= *scotec::TerribleArray<float, *2677, *std::allocator_traits<allocator_type>::rebind_alloc<float>>;\s*"),
         re.compile(r"\s*using *bank_normal_rads *= *std::array<float,3>;\s*"),
         re.compile(r"\s*antennae_per_bank{std::allocator_arg, *allocator, *2677},\s*"),

diff --git a/verification/.devcontainer/devcontainer.json b/verification/.devcontainer/devcontainer.json
@@ -1,6 +1,6 @@
 {
     "name": "C/C++ verification environment",
-    "image": "ghcr.io/opencyphal/toolshed:ts22.4.1",
+    "image": "ghcr.io/opencyphal/toolshed:ts22.4.3",
     "workspaceFolder": "/workspace",
     "workspaceMount": "source=${localWorkspaceFolder}/..,target=/workspace,type=bind,consistency=delegated",
     "mounts": [

diff --git a/verification/CMakeLists.txt b/verification/CMakeLists.txt
@@ -39,7 +39,11 @@ endif()
 set(NUNAVUT_SUBMODULES_ROOT "${NUNAVUT_PROJECT_ROOT}/submodules" CACHE STRING "The path to git submodules for the project.")
 
 if(NOT DEFINED NUNAVUT_VERIFICATION_LANG_STANDARD)
-     set(NUNAVUT_VERIFICATION_LANG_STANDARD "" CACHE STRING "A language standard for languages that support this concept.")
+     set(NUNAVUT_VERIFICATION_LANG_STANDARD "c++20" CACHE STRING "A language standard for languages that support this concept.")
+endif()
+# Ensure NUNAVUT_VERIFICATION_LANG_STANDARD has a default
+if (NUNAVUT_VERIFICATION_LANG STREQUAL "cpp" AND NOT NUNAVUT_VERIFICATION_LANG_STANDARD)
+     set(NUNAVUT_VERIFICATION_LANG_STANDARD "c++20")
 endif()
 
 if(NOT DEFINED NUNAVUT_VERIFICATION_TARGET_ENDIANNESS)
@@ -348,17 +352,6 @@ apply_flag_set("${NUNAVUT_FLAGSET}"
 #   We generate individual test binaires so we can record which test generated
 #   what coverage. We also allow test authors to generate coverage reports for
 #   just one test allowing for faster iteration.
-file(GLOB NATIVE_TESTS_CPP
-     LIST_DIRECTORIES false
-     RELATIVE ${CMAKE_CURRENT_SOURCE_DIR}
-     ${NUNAVUT_VERIFICATION_ROOT}/suite/test_*.cpp
-)
-
-file(GLOB NATIVE_TESTS_C
-     LIST_DIRECTORIES false
-     RELATIVE ${CMAKE_CURRENT_SOURCE_DIR}
-     ${NUNAVUT_VERIFICATION_ROOT}/suite/test_*.c
-)
 
 add_custom_target(
      lcov_zero
@@ -373,16 +366,22 @@ set(ALL_TESTS "")
 set(ALL_TESTS_WITH_LCOV "")
 set(ALL_TEST_COVERAGE "")
 
-foreach(NATIVE_TEST ${NATIVE_TESTS_CPP})
-    get_filename_component(NATIVE_TEST_NAME ${NATIVE_TEST} NAME_WE)
+function(runTestCpp)
+    set(options "")
+    set(oneValueArgs TEST_FILE)
+    set(multiValueArgs LINK LANGUAGE_FLAVORS)
+    cmake_parse_arguments(runTestCpp "${options}" "${oneValueArgs}" "${multiValueArgs}" ${ARGN})
 
     # Skip tests not relevant to the specified language standard
-    if((NATIVE_TEST_NAME STREQUAL "test_array_cetl++14-17" AND NOT NUNAVUT_VERIFICATION_LANG_STANDARD STREQUAL "cetl++14-17")
-          OR
-       (NATIVE_TEST_NAME STREQUAL "test_array_c++17-pmr" AND NOT NUNAVUT_VERIFICATION_LANG_STANDARD STREQUAL "c++17-pmr"))
-        continue()
+    list(FIND runTestCpp_LANGUAGE_FLAVORS "${NUNAVUT_VERIFICATION_LANG_STANDARD}" FIND_INDEX)
+    if (${FIND_INDEX} EQUAL -1)
+        message(STATUS "Skipping ${runTestCpp_TEST_FILE}")
+        return()
     endif()
 
+    set(NATIVE_TEST "${NUNAVUT_VERIFICATION_LANG}/suite/${runTestCpp_TEST_FILE}")
+    get_filename_component(NATIVE_TEST_NAME ${NATIVE_TEST} NAME_WE)
+
     set(${NATIVE_TEST_NAME}_CPP_EXTRA_FLAGS, "")
     list(APPEND ${NATIVE_TEST_NAME}_CPP_EXTRA_FLAGS ${NUNAVUT_VERIFICATION_EXTRA_COMPILE_CFLAGS})
 
@@ -395,22 +394,12 @@ foreach(NATIVE_TEST ${NATIVE_TESTS_CPP})
           list(APPEND ${NATIVE_TEST_NAME}_CPP_EXTRA_FLAGS "-Wno-old-style-cast")
     endif()
 
-    # Link the library that was built for the specified language standard
-    set(TEST_LINK_LIBS "")
-    if((NATIVE_TEST_NAME STREQUAL "test_array_cetl++14-17" AND NUNAVUT_VERIFICATION_LANG_STANDARD STREQUAL "cetl++14-17")
-        OR
-       (NATIVE_TEST_NAME STREQUAL "test_array_c++17-pmr" AND NUNAVUT_VERIFICATION_LANG_STANDARD STREQUAL "c++17-pmr"))
-          set(TEST_LINK_LIBS dsdl-test-array-with-allocator)
-    else()
-          set(TEST_LINK_LIBS dsdl-regulated dsdl-test)
-    endif()
-
     define_native_unit_test("gtest"
                             ${NATIVE_TEST_NAME}
                             ${NATIVE_TEST}
                             ${NUNAVUT_VERIFICATIONS_BINARY_DIR}
                             "${${NATIVE_TEST_NAME}_CPP_EXTRA_FLAGS}"
-                            ${TEST_LINK_LIBS}
+                            ${runTestCpp_LINK}
                             ${LOCAL_ADDITIONAL_DSDL_LIBS}
                             o1heap)
     target_include_directories(${NATIVE_TEST_NAME} PUBLIC "${NUNAVUT_PROJECT_ROOT}/submodules/CETL/include")
@@ -421,9 +410,35 @@ foreach(NATIVE_TEST ${NATIVE_TESTS_CPP})
     list(APPEND ALL_TESTS_WITH_LCOV "run_${NATIVE_TEST_NAME}_with_lcov")
     list(APPEND ALL_TEST_COVERAGE "--add-tracefile")
     list(APPEND ALL_TEST_COVERAGE "${NUNAVUT_VERIFICATIONS_BINARY_DIR}/coverage.${NATIVE_TEST_NAME}.filtered.info")
-endforeach()
+    set(ALL_TESTS ${ALL_TESTS} PARENT_SCOPE)
+    set(ALL_TESTS_WITH_LCOV ${ALL_TESTS_WITH_LCOV} PARENT_SCOPE)
+    set(ALL_TEST_COVERAGE ${ALL_TEST_COVERAGE} PARENT_SCOPE)
+endfunction()
 
-foreach(NATIVE_TEST ${NATIVE_TESTS_C})
+if (NUNAVUT_VERIFICATION_LANG STREQUAL "cpp")
+     runTestCpp(TEST_FILE test_array_c++17-pmr.cpp   LINK dsdl-test-array-with-allocator LANGUAGE_FLAVORS                         c++17-pmr      )
+     runTestCpp(TEST_FILE test_array_cetl++14-17.cpp LINK dsdl-test-array-with-allocator LANGUAGE_FLAVORS       cetl++14-17                      )
+     runTestCpp(TEST_FILE test_bitarray.cpp          LINK dsdl-regulated dsdl-test       LANGUAGE_FLAVORS c++14             c++17 c++17-pmr c++20)
+     runTestCpp(TEST_FILE test_compiles.cpp          LINK dsdl-regulated dsdl-test       LANGUAGE_FLAVORS c++14             c++17 c++17-pmr c++20)
+     runTestCpp(TEST_FILE test_large_bitset.cpp      LINK dsdl-regulated dsdl-test       LANGUAGE_FLAVORS c++14             c++17 c++17-pmr c++20)
+     runTestCpp(TEST_FILE test_serialization.cpp     LINK dsdl-regulated dsdl-test       LANGUAGE_FLAVORS c++14             c++17 c++17-pmr c++20)
+     runTestCpp(TEST_FILE test_unionant.cpp          LINK dsdl-regulated dsdl-test       LANGUAGE_FLAVORS c++14             c++17 c++17-pmr c++20)
+endif()
+
+function(runTestC)
+    set(options "")
+    set(oneValueArgs TEST_FILE)
+    set(multiValueArgs LINK LANGUAGE_FLAVORS)
+    cmake_parse_arguments(runTestC "${options}" "${oneValueArgs}" "${multiValueArgs}" ${ARGN})
+
+    # Skip tests not relevant to the specified language standard
+    list(FIND runTestC_LANGUAGE_FLAVORS "${NUNAVUT_VERIFICATION_LANG_STANDARD}" FIND_INDEX)
+    if (${FIND_INDEX} GREATER -1)
+        message(STATUS "Skipping ${runTestC_TEST_FILE}")
+        return()
+    endif()
+
+    set(NATIVE_TEST "${NUNAVUT_VERIFICATION_ROOT}/suite/${runTestC_TEST_FILE}")
     get_filename_component(NATIVE_TEST_NAME ${NATIVE_TEST} NAME_WE)
     set(${NATIVE_TEST_NAME}_C_EXTRA_FLAGS, "")
     list(APPEND ${NATIVE_TEST_NAME}_C_EXTRA_FLAGS ${NUNAVUT_VERIFICATION_EXTRA_COMPILE_CFLAGS})
@@ -433,16 +448,26 @@ foreach(NATIVE_TEST ${NATIVE_TESTS_C})
                             ${NATIVE_TEST}
                             ${NUNAVUT_VERIFICATIONS_BINARY_DIR}
                             "${${NATIVE_TEST_NAME}_C_EXTRA_FLAGS}"
-                            dsdl-regulated
-                            dsdl-test)
+                            ${runTestC_LINK})
     define_native_test_run(${NATIVE_TEST_NAME} ${NUNAVUT_VERIFICATIONS_BINARY_DIR})
     define_native_test_run_with_lcov(${NATIVE_TEST_NAME} ${NUNAVUT_VERIFICATIONS_BINARY_DIR} ${CMAKE_CURRENT_SOURCE_DIR}/\\*)
     define_natve_test_coverage(${NATIVE_TEST_NAME} ${NUNAVUT_VERIFICATIONS_BINARY_DIR})
     list(APPEND ALL_TESTS "run_${NATIVE_TEST_NAME}")
     list(APPEND ALL_TESTS_WITH_LCOV "run_${NATIVE_TEST_NAME}_with_lcov")
     list(APPEND ALL_TEST_COVERAGE "--add-tracefile")
     list(APPEND ALL_TEST_COVERAGE "${NUNAVUT_VERIFICATIONS_BINARY_DIR}/coverage.${NATIVE_TEST_NAME}.filtered.info")
-endforeach()
+    set(ALL_TESTS ${ALL_TESTS} PARENT_SCOPE)
+    set(ALL_TESTS_WITH_LCOV ${ALL_TESTS_WITH_LCOV} PARENT_SCOPE)
+    set(ALL_TEST_COVERAGE ${ALL_TEST_COVERAGE} PARENT_SCOPE)
+endfunction()
+
+if (NUNAVUT_VERIFICATION_LANG STREQUAL "c")
+     runTestCpp(TEST_FILE test_canard.cpp                         LINK dsdl-regulated dsdl-test LANGUAGE_FLAVORS c11)
+     runTestC(  TEST_FILE test_constant.c                         LINK dsdl-regulated dsdl-test LANGUAGE_FLAVORS c11)
+     runTestC(  TEST_FILE test_override_variable_array_capacity.c LINK dsdl-regulated dsdl-test LANGUAGE_FLAVORS c11)
+     runTestC(  TEST_FILE test_serialization.c                    LINK dsdl-regulated dsdl-test LANGUAGE_FLAVORS c11)
+     runTestC(  TEST_FILE test_support.c                          LINK dsdl-regulated dsdl-test LANGUAGE_FLAVORS c11)
+endif()
 
 # +---------------------------------------------------------------------------+
 #   Finally, we setup an overall report. the coverage.info should be uploaded

diff --git a/verification/cpp/suite/test_array_c++17-pmr.cpp b/verification/cpp/suite/test_array_c++17-pmr.cpp
@@ -128,3 +128,23 @@ TEST(StdVectorTests, SerializationRoundtrip) {
     ASSERT_EQ(outer2.outer_primitive, 777777);
 
 }
+
+/**
+ * Verify that std::pmr::polymorphic_allocator gets passed down to nested types
+ */
+TEST(StdVectorTests, TestAllocatorIsPassedDown) {
+
+    ASSERT_TRUE((std::uses_allocator<mymsgs::OuterMore_1_0, std::pmr::polymorphic_allocator<void>>::value));
+
+    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<void> pa{&mbr};
+
+    mymsgs::OuterMore_1_0 outer;
+    outer.outer_items.push_back(1.23456f);
+    outer.inners.resize(1);
+    outer.inners[0].inner_items.resize(1);
+
+    // Verify that the allocator got passed down from the OuterMore_1_0 to the InnerMore_1_0
+    ASSERT_EQ(outer.inners[0].inner_items.get_allocator().resource(), outer.outer_items.get_allocator().resource());
+}
diff --git a/verification/cpp/suite/test_array_cetl++14-17.cpp b/verification/cpp/suite/test_array_cetl++14-17.cpp
@@ -126,3 +126,23 @@ TEST(CetlVlaPmrTests, SerializationRoundtrip) {
     ASSERT_EQ(outer2.inners[1].inner_primitive, true);
     ASSERT_EQ(outer2.outer_primitive, 777777);
 }
+
+/**
+ * Verify that cetl::pf17::pmr::polymorphic_allocator gets passed down to nested types
+ */
+TEST(CetlVlaPmrTests, TestAllocatorIsPassedDown) {
+
+    ASSERT_TRUE((std::uses_allocator<mymsgs::OuterMore_1_0, cetl::pf17::pmr::polymorphic_allocator<void>>::value));
+
+    std::array<cetl::pf17::byte, 500> buffer{};
+    cetl::pf17::pmr::monotonic_buffer_resource mbr{buffer.data(), buffer.size(), cetl::pf17::pmr::null_memory_resource()};
+    cetl::pf17::pmr::polymorphic_allocator<void> pa{&mbr};
+
+    mymsgs::OuterMore_1_0 outer{pa};
+    outer.outer_items.push_back(1.23456f);
+    outer.inners.resize(1);
+    outer.inners[0].inner_items.resize(1);
+
+    // Verify that the allocator got passed down from the OuterMore_1_0 to the InnerMore_1_0
+    ASSERT_EQ(outer.inners[0].inner_items.get_allocator().resource(), outer.outer_items.get_allocator().resource());
+}