From 643625c757531fb03f9205aa0263e4c0a4df81dd Mon Sep 17 00:00:00 2001 From: Skeets Norquist Date: Tue, 29 Aug 2023 11:11:13 -0700 Subject: [PATCH] Simplify message class down to a non-template plus other refinements --- docs/languages.rst | 2 + src/nunavut/lang/__init__.py | 8 +- src/nunavut/lang/_language.py | 2 + src/nunavut/lang/cpp/__init__.py | 102 ++++++------------ .../lang/cpp/templates/_composite_type.j2 | 53 ++++----- src/nunavut/lang/cpp/templates/_fields.j2 | 2 +- src/nunavut/lang/properties.yaml | 3 + test/gentest_arrays/test_arrays.py | 5 +- test/gettest_properties/test_properties.py | 6 +- 9 files changed, 69 insertions(+), 114 deletions(-) diff --git a/docs/languages.rst b/docs/languages.rst index 5ce7df57..7be531f0 100644 --- a/docs/languages.rst +++ b/docs/languages.rst @@ -36,6 +36,7 @@ c++17-pmr.yaml variable_array_type_constructor_args: "" allocator_include: "" allocator_type: "std::pmr::polymorphic_allocator" + allocator_is_default_constructible: true ctor_convention: "uses-trailing-allocator" cetl++.yaml @@ -50,6 +51,7 @@ cetl++.yaml variable_array_type_constructor_args: "{MAX_SIZE}" allocator_include: '"cetl/pf17/sys/memory_resource.hpp"' allocator_type: "cetl::pf17::pmr::polymorphic_allocator" + allocator_is_default_constructible: false ctor_convention: "uses-trailing-allocator" nnvg command diff --git a/src/nunavut/lang/__init__.py b/src/nunavut/lang/__init__.py index 8efb7200..32d10ed3 100644 --- a/src/nunavut/lang/__init__.py +++ b/src/nunavut/lang/__init__.py @@ -19,6 +19,8 @@ logger = logging.getLogger(__name__) +NUNAVUT_LANG_CPP = "nunavut.lang.cpp" + class UnsupportedLanguageError(ValueError): """ @@ -223,9 +225,9 @@ def set_target_language(self, target_language: typing.Optional[str]) -> "Languag def load_default_config(self, language_standard: str) -> None: self._ln_loader.config # Accessing this property causes the defaults to load defaults_key = f"{language_standard}_options" - if defaults_key in self._ln_loader.config.sections()["nunavut.lang.cpp"]: - defaults_data = self._ln_loader.config.get_config_value_as_dict("nunavut.lang.cpp", defaults_key) - self._ln_loader.config.update_section("nunavut.lang.cpp", {"options": defaults_data}) + if defaults_key in self._ln_loader.config.sections()[NUNAVUT_LANG_CPP]: + defaults_data = self._ln_loader.config.get_config_value_as_dict(NUNAVUT_LANG_CPP, defaults_key) + self._ln_loader.config.update_section(NUNAVUT_LANG_CPP, {"options": defaults_data}) def set_additional_config_files( self, additional_config_files: typing.List[pathlib.Path] diff --git a/src/nunavut/lang/_language.py b/src/nunavut/lang/_language.py index e1058af7..05cf96b8 100644 --- a/src/nunavut/lang/_language.py +++ b/src/nunavut/lang/_language.py @@ -121,6 +121,7 @@ def __init__(self, language_module_name: str, config: LanguageConfig, **kwargs: self._validate_language_options(self._language_options) def _validate_language_options(self, language_options: typing.Mapping[str, typing.Any]) -> None: + """Subclasses may override this method to validate language-specific options""" pass def __getattr__(self, name: str) -> typing.Any: @@ -228,6 +229,7 @@ def named_values(self) -> typing.Mapping[str, str]: # +-----------------------------------------------------------------------+ def _add_additional_globals(self, globals_map: typing.Dict[str, typing.Any]) -> None: + """Subclasses may override this method to populate additional language-specific globals""" pass def get_support_module(self) -> typing.Tuple[str, typing.Tuple[int, int, int], typing.Optional["types.ModuleType"]]: diff --git a/src/nunavut/lang/cpp/__init__.py b/src/nunavut/lang/cpp/__init__.py index 6771a0de..a58a869a 100644 --- a/src/nunavut/lang/cpp/__init__.py +++ b/src/nunavut/lang/cpp/__init__.py @@ -257,15 +257,15 @@ def do_includes_test(override_vla_include, override_allocator_include): std_includes.append("variant") includes_formatted = ["<{}>".format(include) for include in sorted(std_includes)] + allocator_include = str(self.get_option("allocator_include", "")) + if len(allocator_include) > 0: + includes_formatted.append(allocator_include) + if dep_types.uses_variable_length_array: variable_array_include = str(self.get_option("variable_array_type_include", "")) if len(variable_array_include) > 0: includes_formatted.append(variable_array_include) - allocator_include = str(self.get_option("allocator_include", "")) - if len(allocator_include) > 0: - includes_formatted.append(allocator_include) - return includes_formatted def filter_id(self, instance: typing.Any, id_type: str = "any") -> str: @@ -273,24 +273,6 @@ def filter_id(self, instance: typing.Any, id_type: str = "any") -> str: return self._get_token_encoder().strop(raw_name, id_type) - def filter_short_reference_impl_name( - self, t: pydsdl.CompositeType, stropping: YesNoDefault = YesNoDefault.DEFAULT, id_type: str = "any" - ) -> str: - """ - Formulate the type name. If an allocator is being used (ctor_convention != "default") then it will be - suffixed with "_Impl" and a type alias will define the concrete type with template arguments filled in. - - :param pydsdl.CompositeType t: The DSDL type to get the reference name for. - :param YesNoDefault stropping: If DEFAULT then the stropping value configured for the target language is used - else this overrides that value. - :param str id_type: A type of identifier. This is different for each language. For example, for C this - value can be 'typedef', 'macro', 'function', or 'enum'. - Use 'any' to apply stropping rules for all identifier types to the instance. - """ - name: str = self.filter_short_reference_name(t, stropping, id_type) - suffix: str = "" if self.get_option("ctor_convention") == ConstructorConvention.Default.value else "_Impl" - return name + suffix - def create_bitset_decl(self, type: str, max_size: int) -> str: return "std::bitset<{MAX_SIZE}>".format(MAX_SIZE=max_size) @@ -301,7 +283,7 @@ def create_vla_decl(self, type: str, max_size: int) -> str: variable_array_type_template = self.get_option("variable_array_type_template") if not isinstance(variable_array_type_template, str) or len(variable_array_type_template) == 0: raise RuntimeError("You must specify a value for the 'variable_array_type_template' option.") - rebind_allocator = "typename std::allocator_traits::template rebind_alloc<{TYPE}>".format(TYPE=type) + rebind_allocator = "std::allocator_traits::rebind_alloc<{TYPE}>".format(TYPE=type) return variable_array_type_template.format(TYPE=type, MAX_SIZE=max_size, REBIND_ALLOCATOR=rebind_allocator) @@ -871,16 +853,6 @@ def filter_short_reference_name(language: Language, t: pydsdl.CompositeType) -> return language.filter_short_reference_name(t) -@template_language_filter(__name__) -def filter_short_reference_impl_name(language: Language, t: pydsdl.CompositeType) -> str: - if isinstance(t, pydsdl.ServiceType): - if YesNoDefault.test_truth(YesNoDefault.DEFAULT, language.enable_stropping): - return language.filter_id(t.short_name) - else: - return str(t.short_name) - return language.filter_short_reference_impl_name(t) - - @template_language_list_filter(__name__) @template_environment_list_filter def filter_includes( @@ -994,32 +966,38 @@ def filter_default_value_initializer(language: Language, instance: pydsdl.Any) - return "" -@template_language_filter(__name__) -def filter_value_initializer( # noqa: C901 disable warning about function being too complex - language: Language, instance: pydsdl.Any, special_method: SpecialMethod -) -> str: - """ - Emit an initialization expression for a C++ special method. - """ +def needs_rhs(special_method: SpecialMethod) -> bool: + """Helper method used by filter_value_initializer()""" + return special_method in ( + SpecialMethod.CopyConstructorWithAllocator, + SpecialMethod.MoveConstructorWithAllocator, + ) - def needs_rhs(special_method: SpecialMethod) -> bool: - return special_method in ( - SpecialMethod.CopyConstructorWithAllocator, - SpecialMethod.MoveConstructorWithAllocator, - ) - def needs_allocator(instance: pydsdl.Any) -> bool: - return isinstance(instance.data_type, pydsdl.VariableLengthArrayType) or isinstance( - instance.data_type, pydsdl.CompositeType - ) +def needs_allocator(instance: pydsdl.Any) -> bool: + """Helper method used by filter_value_initializer()""" + return isinstance(instance.data_type, pydsdl.VariableLengthArrayType) or isinstance( + instance.data_type, pydsdl.CompositeType + ) - def needs_vla_init_args(instance: pydsdl.Any, special_method: SpecialMethod) -> bool: - return special_method == SpecialMethod.DefaultConstructorWithOptionalAllocator and isinstance( - instance.data_type, pydsdl.VariableLengthArrayType - ) - def needs_move(special_method: SpecialMethod) -> bool: - return special_method == SpecialMethod.MoveConstructorWithAllocator +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( + instance.data_type, pydsdl.VariableLengthArrayType + ) + + +def needs_move(special_method: SpecialMethod) -> bool: + """Helper method used by filter_value_initializer()""" + return special_method == SpecialMethod.MoveConstructorWithAllocator + + +@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. + """ if ( isinstance(instance.data_type, pydsdl.PrimitiveType) @@ -1059,20 +1037,6 @@ def needs_move(special_method: SpecialMethod) -> bool: return "" -@template_language_filter(__name__) -def filter_field_declaration(language: Language, instance: pydsdl.Any) -> str: - """ - Emit a field declaration that accounts for the message being a templated type - """ - suffix: str = "" - if ( - isinstance(instance, pydsdl.CompositeType) - and language.get_option("ctor_convention") != ConstructorConvention.Default.value - ): - suffix = "_Impl" - return filter_declaration(language, instance) + suffix - - @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 72c3d32d..6c25542c 100644 --- a/src/nunavut/lang/cpp/templates/_composite_type.j2 +++ b/src/nunavut/lang/cpp/templates/_composite_type.j2 @@ -31,27 +31,29 @@ {% if composite_type.deprecated -%} [[deprecated("{{ composite_type }} is reaching the end of its life; there may be a newer version available")]] {% endif -%} -{% if options.ctor_convention != ConstructorConvention.Default.value -%} -template > -{% endif -%} -struct {{composite_type|short_reference_impl_name}} final +struct {{composite_type|short_reference_name}} final { {%- if options.ctor_convention != ConstructorConvention.Default.value %} + using allocator_type = {{ options.allocator_type }}; + // Default constructor - {{composite_type|short_reference_impl_name}}(const Allocator& allocator = Allocator()){%if composite_type.fields_except_padding %} :{%endif%} + {{composite_type|short_reference_name}}(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.DefaultConstructorWithOptionalAllocator) }}{%if not loop.last %},{%endif %} {%- endfor %} { } // Destructor - ~{{composite_type|short_reference_impl_name}}() = default; + ~{{composite_type|short_reference_name}}() = default; // Copy constructor - {{composite_type|short_reference_impl_name}}(const {{composite_type|short_reference_impl_name}}&) = default; + {{composite_type|short_reference_name}}(const {{composite_type|short_reference_name}}&) = default; // Copy constructor with allocator - {{composite_type|short_reference_impl_name}}(const {{composite_type|short_reference_impl_name}}& rhs, const Allocator& allocator){%if composite_type.fields_except_padding %} :{%endif%} + {{composite_type|short_reference_name}}(const {{composite_type|short_reference_name}}& rhs, const allocator_type& allocator) + {%- if composite_type.fields_except_padding %} :{% endif %} {%- for field in composite_type.fields_except_padding %} {{ field | id }}{{ field | value_initializer(SpecialMethod.CopyConstructorWithAllocator) }}{%if not loop.last %},{%endif %} {%- endfor %} @@ -59,10 +61,11 @@ struct {{composite_type|short_reference_impl_name}} final } // Move constructor - {{composite_type|short_reference_impl_name}}({{composite_type|short_reference_impl_name}}&&) = default; + {{composite_type|short_reference_name}}({{composite_type|short_reference_name}}&&) = default; // Move constructor with allocator - {{composite_type|short_reference_impl_name}}({{composite_type|short_reference_impl_name}}&& rhs, const Allocator& allocator){%if composite_type.fields_except_padding %} :{%endif%} + {{composite_type|short_reference_name}}({{composite_type|short_reference_name}}&& rhs, const allocator_type& allocator) + {%- if composite_type.fields_except_padding %} :{% endif %} {%- for field in composite_type.fields_except_padding %} {{ field | id }}{{ field | value_initializer(SpecialMethod.MoveConstructorWithAllocator) }}{%if not loop.last %},{%endif %} {%- endfor %} @@ -70,10 +73,10 @@ struct {{composite_type|short_reference_impl_name}} final } // Copy assignment - {{composite_type|short_reference_impl_name}}& operator=(const {{composite_type|short_reference_impl_name}}&) = default; + {{composite_type|short_reference_name}}& operator=(const {{composite_type|short_reference_name}}&) = default; // Move assignment - {{composite_type|short_reference_impl_name}}& operator=({{composite_type|short_reference_impl_name}}&&) = default; + {{composite_type|short_reference_name}}& operator=({{composite_type|short_reference_name}}&&) = default; {%- endif %} struct _traits_ // The name is surrounded with underscores to avoid collisions with DSDL attributes. @@ -117,7 +120,7 @@ struct {{composite_type|short_reference_impl_name}} final { TypeOf() = delete; {%- endif %} - using {{field.name|id}} = {{ field.data_type | field_declaration }}; + using {{field.name|id}} = {{ field.data_type | declaration }}; {%- if loop.last %} }; {%- endif %} @@ -172,33 +175,15 @@ struct {{composite_type|short_reference_impl_name}} final {%- endif %} }; -{% if options.ctor_convention != ConstructorConvention.Default.value -%} -using {{composite_type|short_reference_name}} = {{composite_type|short_reference_impl_name}}>; -{% endif -%} - -{% if not nunavut.support.omit -%} - -{%- macro message_arg_type(message_name) -%} -{{message_name}} -{%- if options.ctor_convention != ConstructorConvention.Default.value -%} - -{%- endif -%} -{%- endmacro -%} - -{% if options.ctor_convention != ConstructorConvention.Default.value %} -template -{% endif -%} -inline nunavut::support::SerializeResult serialize(const {{ message_arg_type(composite_type|short_reference_impl_name) }}& obj, +{% if not nunavut.support.omit %} +inline nunavut::support::SerializeResult serialize(const {{composite_type|short_reference_name}}& obj, nunavut::support::bitspan out_buffer) { {% from 'serialization.j2' import serialize -%} {{ serialize(composite_type) | trim | remove_blank_lines }} } -{% if options.ctor_convention != ConstructorConvention.Default.value -%} -template -{% endif -%} -inline nunavut::support::SerializeResult deserialize({{ message_arg_type(composite_type|short_reference_impl_name) }}& obj, +inline nunavut::support::SerializeResult deserialize({{composite_type|short_reference_name}}& obj, nunavut::support::const_bitspan in_buffer) { {% from 'deserialization.j2' import deserialize -%} diff --git a/src/nunavut/lang/cpp/templates/_fields.j2 b/src/nunavut/lang/cpp/templates/_fields.j2 index 4faeb56b..d9c02496 100644 --- a/src/nunavut/lang/cpp/templates/_fields.j2 +++ b/src/nunavut/lang/cpp/templates/_fields.j2 @@ -11,7 +11,7 @@ {% endif -%} {{ field.doc | block_comment('cpp-doxygen', 4, 120) }} {% if options.ctor_convention != ConstructorConvention.Default.value -%} - typename _traits_::TypeOf::{{field.name|id}} {{ field | id }}; + _traits_::TypeOf::{{field.name|id}} {{ field | id }}; {%- else -%} _traits_::TypeOf::{{field.name|id}} {{ field | id }}{{ field.data_type | default_value_initializer }}; {%- endif -%} diff --git a/src/nunavut/lang/properties.yaml b/src/nunavut/lang/properties.yaml index ff4f60dd..7dcec101 100644 --- a/src/nunavut/lang/properties.yaml +++ b/src/nunavut/lang/properties.yaml @@ -318,6 +318,7 @@ nunavut.lang.cpp: variable_array_type_constructor_args: "" allocator_include: "" allocator_type: "" + allocator_is_default_constructible: true ctor_convention: "default" cetl++_options: variable_array_type_include: '"cetl/variable_length_array.hpp"' @@ -325,6 +326,7 @@ nunavut.lang.cpp: variable_array_type_constructor_args: "{MAX_SIZE}" allocator_include: '"cetl/pf17/sys/memory_resource.hpp"' allocator_type: "cetl::pf17::pmr::polymorphic_allocator" + allocator_is_default_constructible: false ctor_convention: "uses-trailing-allocator" c++17-pmr_options: variable_array_type_include: "" @@ -332,6 +334,7 @@ nunavut.lang.cpp: variable_array_type_constructor_args: "" allocator_include: "" 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 index 07439909..b7db2dc4 100644 --- a/test/gentest_arrays/test_arrays.py +++ b/test/gentest_arrays/test_arrays.py @@ -62,6 +62,7 @@ def test_var_array_override_cpp(gen_paths): # type: ignore "variable_array_type_constructor_args": "{MAX_SIZE}", "allocator_include": '"scotec/alloc.hpp"', "allocator_type": "TerribleAllocator", + "allocator_is_default_constructible": True, "ctor_convention": "uses-leading-allocator" } root_namespace = str(gen_paths.dsdl_dir / Path("radar")) @@ -81,8 +82,8 @@ 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".*\s+class Allocator = TerribleAllocator"), - re.compile(r"\s*using *antennae_per_bank *= *scotec::TerribleArray::template rebind_alloc>;\s*"), + re.compile(r".*\bconst allocator_type& allocator = allocator_type()"), + re.compile(r"\s*using *antennae_per_bank *= *scotec::TerribleArray::rebind_alloc>;\s*"), re.compile(r"\s*using *bank_normal_rads *= *std::array;\s*"), re.compile(r"\s*antennae_per_bank{std::allocator_arg, *allocator, *2677},\s*"), diff --git a/test/gettest_properties/test_properties.py b/test/gettest_properties/test_properties.py index 4324efcd..9551ef2f 100644 --- a/test/gettest_properties/test_properties.py +++ b/test/gettest_properties/test_properties.py @@ -29,13 +29,13 @@ def test_issue_277(gen_paths): # type: ignore "variable_array_type_constructor_args": "g_bad_global_thing", "allocator_include": '"MyCrazyAllocator.hpp"', "allocator_type": "MyCrazyAllocator", + "allocator_is_default_constructible": True, "ctor_convention": "uses-leading-allocator" } vla_decl_pattern = re.compile(r"\b|^MyCrazyArray\B") vla_include_pattern = re.compile(r"^#include\s+\"MyCrazyArray\.hpp\"$") alloc_include_pattern = re.compile(r"^#include\s+\"MyCrazyAllocator\.hpp\"$") - alloc_decl_pattern = re.compile(r".*\bclass Allocator = MyCrazyAllocator\b") vla_constructor_args_pattern = re.compile(r".*\bstd::allocator_arg, allocator, g_bad_global_thing\b") overrides_file = gen_paths.out_dir / pathlib.Path("overrides_test_issue_277.yaml") @@ -68,7 +68,6 @@ def test_issue_277(gen_paths): # type: ignore found_vla_decl = False found_vla_include = False found_alloc_include = False - found_alloc_decl = False found_vla_constructor_args = False with open(str(outfile), "r") as header_file: for line in header_file: @@ -78,13 +77,10 @@ def test_issue_277(gen_paths): # type: ignore found_vla_include = True if not found_alloc_include and alloc_include_pattern.search(line): found_alloc_include = True - if not found_alloc_decl and alloc_decl_pattern.search(line): - found_alloc_decl = True if not found_vla_constructor_args and vla_constructor_args_pattern.search(line): found_vla_constructor_args = True assert found_vla_decl assert found_vla_include assert found_alloc_include - assert found_alloc_decl assert found_vla_constructor_args