Skip to content

Commit

Permalink
Fix issues: allocator passing, verification test C++ flavors, union c…
Browse files Browse the repository at this point in the history
…ode-gen

- 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.
  • Loading branch information
skeetsaz committed Nov 18, 2023
1 parent 7ecfd68 commit db157b4
Show file tree
Hide file tree
Showing 6 changed files with 89 additions and 17 deletions.
35 changes: 27 additions & 8 deletions src/nunavut/lang/cpp/templates/_composite_type.j2
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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
Expand All @@ -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 %}
Expand All @@ -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 %}
Expand All @@ -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
Expand All @@ -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
Expand Down
2 changes: 1 addition & 1 deletion src/nunavut/lang/properties.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down
2 changes: 1 addition & 1 deletion verification/.devcontainer/devcontainer.json
Original file line number Diff line number Diff line change
@@ -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": [
Expand Down
27 changes: 20 additions & 7 deletions verification/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -373,16 +373,21 @@ 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 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()
if(NOT ${NUNAVUT_VERIFICATION_LANG_STANDARD} IN_LIST runTestCpp_LANGUAGE_FLAVORS)
message(STATUS "Skipping ${runTestCpp_TEST_FILE}")
return()
endif()

set(NATIVE_TEST "${NUNAVUT_VERIFICATION_ROOT}/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})

Expand Down Expand Up @@ -421,7 +426,15 @@ 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()
endfunction()

runTestCpp(TEST_FILE test_array_c++17-pmr.cpp LANGUAGE_FLAVORS c++17-pmr )
runTestCpp(TEST_FILE test_array_cetl++14-17.cpp LANGUAGE_FLAVORS cetl++14-17 )
runTestCpp(TEST_FILE test_bitarray.cpp LANGUAGE_FLAVORS c++14 cetl++14-17 c++17 c++17-pmr c++20)
runTestCpp(TEST_FILE test_compiles.cpp LANGUAGE_FLAVORS c++14 c++17 c++17-pmr c++20)
runTestCpp(TEST_FILE test_large_bitset.cpp LANGUAGE_FLAVORS c++14 c++17 c++17-pmr c++20)
runTestCpp(TEST_FILE test_serialization.cpp LANGUAGE_FLAVORS c++14 c++17 c++17-pmr c++20)
runTestCpp(TEST_FILE test_unionant.cpp LANGUAGE_FLAVORS c++14 c++17 c++17-pmr c++20)

foreach(NATIVE_TEST ${NATIVE_TESTS_C})
get_filename_component(NATIVE_TEST_NAME ${NATIVE_TEST} NAME_WE)
Expand Down
20 changes: 20 additions & 0 deletions verification/cpp/suite/test_array_c++17-pmr.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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());
}
20 changes: 20 additions & 0 deletions verification/cpp/suite/test_array_cetl++14-17.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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());
}

0 comments on commit db157b4

Please sign in to comment.