Fix formatting inconsistencies in the C code generator (not C++) (Ope…

…nCyphal#291) - Closes OpenCyphal#178 - Also, fix benign warnings about uninitialized reads in C++ `VariableLengthArray<bool>`
edu-rossrobotics · Mar 6, 2023 · 609f7ee · 609f7ee
1 parent 9dafc1c
commit 609f7ee
Show file tree

Hide file tree

Showing 5 changed files with 60 additions and 53 deletions.
diff --git a/src/nunavut/_version.py b/src/nunavut/_version.py
@@ -8,7 +8,7 @@
 .. autodata:: __version__
 """
 
-__version__ = "2.0.5"
+__version__ = "2.0.6"
 __license__ = "MIT"
 __author__ = "OpenCyphal"
 __copyright__ = "Copyright 2022 Amazon.com, Inc. or its affiliates. All Rights Reserved. Copyright (c) 2022 OpenCyphal."

diff --git a/src/nunavut/lang/c/templates/definitions.j2 b/src/nunavut/lang/c/templates/definitions.j2
@@ -14,37 +14,43 @@
 {% endif %}
 
 {# ----------------------------------------------------------------------------------------------------------------- #}
-{% macro generate_metadata(t) %}
-{% set ref = t | full_reference_name %}
+{% macro generate_metadata(t) -%}
+// +-------------------------------------------------------------------------------------------------------------------+
+// | {{ t }}
+// +-------------------------------------------------------------------------------------------------------------------+
+{%- set ref =t|full_reference_name%}
 #define {{ ref }}_FULL_NAME_             "{{ t.full_name }}"
 #define {{ ref }}_FULL_NAME_AND_VERSION_ "{{ t.full_name }}.{{ t.version.major }}.{{ t.version.minor }}"
-{% endmacro %}
-
-{# ----------------------------------------------------------------------------------------------------------------- #}
-{% macro generate_composite(t) %}
+{%- if t is not ServiceType %}
+{%- assert t.extent % 8 == 0 %}
+{%- assert t.inner_type.extent % 8 == 0 %}
 
-{{ generate_metadata(t) }}
-
-{% assert t.extent % 8 == 0 %}
-{% assert t.inner_type.extent % 8 == 0 %}
 /// Extent is the minimum amount of memory required to hold any serialized representation of any compatible
 /// version of the data type; or, on other words, it is the the maximum possible size of received objects of this type.
 /// The size is specified in bytes (rather than bits) because by definition, extent is an integer number of bytes long.
 /// When allocating a deserialization (RX) buffer for this data type, it should be at least extent bytes large.
 /// When allocating a serialization (TX) buffer, it is safe to use the size of the largest serialized representation
 /// instead of the extent because it provides a tighter bound of the object size; it is safe because the concrete type
 /// is always known during serialization (unlike deserialization). If not sure, use extent everywhere.
-#define {{ t | full_reference_name }}_EXTENT_BYTES_                    {{ t.extent // 8 }}UL
-#define {{ t | full_reference_name }}_SERIALIZATION_BUFFER_SIZE_BYTES_ {{ t.inner_type.extent // 8 }}UL
-static_assert({{ t | full_reference_name }}_EXTENT_BYTES_ >= {# -#}
-              {{ t | full_reference_name }}_SERIALIZATION_BUFFER_SIZE_BYTES_,
+#define {{ ref }}_EXTENT_BYTES_                    {{ t.extent // 8 }}UL
+#define {{ ref }}_SERIALIZATION_BUFFER_SIZE_BYTES_ {{ t.inner_type.extent // 8 }}UL
+static_assert({{ ref }}_EXTENT_BYTES_ >= {{ ref }}_SERIALIZATION_BUFFER_SIZE_BYTES_,
               "Internal constraint violation");
+{%- endif %}
+{% endmacro %}
+
+{# ----------------------------------------------------------------------------------------------------------------- #}
+{% macro generate_composite(t) %}
+{{ generate_metadata(t) }}
+
+{%- for constant in t.constants %}
 
-{% for constant in t.constants %}
 /// {{ constant }}
 #define {{ t | full_reference_name }}_{{ constant.name }} ({{ constant | constant_value }})
 {%- endfor %}
-{% for f in t.fields_except_padding if f.data_type is ArrayType %}
+
+{%- for f in t.fields_except_padding if f.data_type is ArrayType %}
+
 /// Array metadata for: {{ f }}
 {%- if options.enable_override_variable_array_capacity and f.data_type is VariableLengthArrayType %}
 #ifndef {{ t | full_reference_name }}_{{ f.name }}_ARRAY_CAPACITY_
@@ -61,19 +67,13 @@ static_assert({{ t | full_reference_name }}_EXTENT_BYTES_ >= {# -#}
 #define {{ t | full_reference_name }}_{{ f.name }}_ARRAY_IS_VARIABLE_LENGTH_ {# -#}
         {{ valuetoken_true if f.data_type is VariableLengthArrayType else valuetoken_false }}
 {%- endfor %}
-{% if t.inner_type is UnionType %}
-/// The number of fields in the union. Valid tag values range from zero to this value minus one, inclusive.
-#define {{ t | full_reference_name }}_UNION_OPTION_COUNT_ {{ t.fields | length }}U
-{% endif %}
 
-{% if t.inner_type is StructureType %}
+{%- if t.inner_type is StructureType %}
 {{ _define_structure(t.inner_type) }}
-
-{% elif t.inner_type is UnionType %}
+{%- elif t.inner_type is UnionType %}
 {{ _define_union(t.inner_type) }}
-
-{% else %}{% assert False %}{# Not a valid composite type. #}
-{% endif %}
+{%- else %}{% assert False %}{# Not a valid composite type. #}
+{%- endif %}
 
 {{ _define_functions(t) }}
 
@@ -123,6 +123,9 @@ typedef struct
     };
     {{ t.tag_field_type | type_from_primitive }} _tag_;
 } {{ t | full_reference_name }};
+
+/// The number of fields in the union. Valid tag values range from zero to this value minus one, inclusive.
+#define {{ t | full_reference_name }}_UNION_OPTION_COUNT_ {{ t.fields | length }}U
 {% endmacro %}
 
 
@@ -166,9 +169,7 @@ struct  /// Array address equivalence guarantee: &elements[0] == &{{ name }}
 
 {# ----------------------------------------------------------------------------------------------------------------- #}
 {% macro _define_functions(t) %}
-
-{% if not nunavut.support.omit %}
-
+{%- if not nunavut.support.omit %}
 /// Serialize an instance into the provided buffer.
 /// The lifetime of the resulting serialized representation is independent of the original instance.
 /// This method may be slow for large objects (e.g., images, point clouds, radar samples), so in a later revision
@@ -191,7 +192,7 @@ static inline {{ typename_error_type }} {{ t | full_reference_name }}_serialize_
     {{ typename_unsigned_length }}* const inout_buffer_size_bytes)
 {
     {% from 'serialization.j2' import serialize -%}
-    {{ serialize(t) | trim }}
+    {{ serialize(t)|trim|remove_blank_lines }}
 }
 
 /// Deserialize an instance from the provided buffer.
@@ -219,7 +220,7 @@ static inline {{ typename_error_type }} {{ t | full_reference_name }}_deserializ
     {{ typename_unsigned_length }}* const inout_buffer_size_bytes)
 {
     {% from 'deserialization.j2' import deserialize -%}
-    {{ deserialize(t) | trim }}
+    {{ deserialize(t)|trim|remove_blank_lines }}
 }
 
 /// Initialize an instance to default values. Does nothing if @param out_obj is {{ valuetoken_null }}.
@@ -238,10 +239,10 @@ static inline void {{ t | full_reference_name }}_initialize_({{ t | full_referen
     }
 }
 
-{% endif %}  {# if not nunavut.support.omit #}
+{%- endif %}  {# if not nunavut.support.omit #}
 
-{% for f in t.fields_except_padding %}
-{% if t.inner_type is UnionType %}
+{%- for f in t.fields_except_padding %}
+{%- if t.inner_type is UnionType %}
 /// Mark option "{{ f.name }}" active without initializing it. Does nothing if @param obj is {{ valuetoken_null }}.
 static inline void {{ t | full_reference_name }}_select_{{ f.name }}_{# -#}
                   ({{ t | full_reference_name }}* const obj)
@@ -258,6 +259,6 @@ static inline {{ typename_boolean }} {{ t | full_reference_name }}_is_{{ f.name
 {
     return ((obj != {{ valuetoken_null }}) && (obj->_tag_ == {{ loop.index0 }}));
 }
-{% endif %}
+{%- endif %}
 {% endfor %}
 {% endmacro %}
diff --git a/src/nunavut/lang/c/templates/deserialization.j2 b/src/nunavut/lang/c/templates/deserialization.j2
@@ -20,11 +20,11 @@
     {
         buffer = (const {{ typename_byte }}*)"";
     }
-{% if t.inner_type.bit_length_set.max > 0 %}
-    {{ _deserialize_impl(t) }}
-{% else %}
+{%- if t.inner_type.bit_length_set.max > 0 %}
+    {{ _deserialize_impl(t)|remove_blank_lines }}
+{%- else %}
     *inout_buffer_size_bytes = 0U;
-{% endif %}
+{%- endif %}
     return NUNAVUT_SUCCESS;
 {% endmacro %}
 
@@ -42,16 +42,16 @@
     {{ _pad_to_alignment(f.data_type.alignment_requirement) }}
         {% endif %}
     // {{ f }}
-    {{ _deserialize_any(f.data_type, 'out_obj->' + (f|id), offset)|trim|remove_blank_lines }}
+    {{ _deserialize_any(f.data_type, 'out_obj->' + (f|id), offset)|trim }}
     {% endfor %}
 {% elif t.inner_type is UnionType %}
     // Union tag field: {{ t.inner_type.tag_field_type }}
-    {{ _deserialize_integer(t.inner_type.tag_field_type, 'out_obj->_tag_', 0|bit_length_set)|trim|remove_blank_lines }}
+    {{ _deserialize_integer(t.inner_type.tag_field_type, 'out_obj->_tag_', 0|bit_length_set)|trim }}
     {% for f, offset in t.inner_type.iterate_fields_with_offsets() %}
     {{ 'if' if loop.first else 'else if' }} ({{ loop.index0 }}U == out_obj->_tag_)  // {{ f }}
     {
         {%- assert f.data_type.alignment_requirement <= (offset.min) %}
-        {{ _deserialize_any(f.data_type, 'out_obj->' + (f|id), offset)|trim|remove_blank_lines|indent }}
+        {{ _deserialize_any(f.data_type, 'out_obj->' + (f|id), offset)|trim|indent }}
     }
     {%- endfor %}
     else

diff --git a/src/nunavut/lang/c/templates/serialization.j2 b/src/nunavut/lang/c/templates/serialization.j2
@@ -16,11 +16,11 @@
     {
         return -NUNAVUT_ERROR_INVALID_ARGUMENT;
     }
-{% if t.inner_type.bit_length_set.max > 0 %}
-    {{ _serialize_impl(t) }}
-{% else %}
+{%- if t.inner_type.bit_length_set.max > 0 %}
+    {{ _serialize_impl(t)|remove_blank_lines }}
+{%- else %}
     *inout_buffer_size_bytes = 0U;
-{% endif %}
+{%- endif %}
     return NUNAVUT_SUCCESS;
 {% endmacro %}
 
@@ -46,24 +46,24 @@
         {% if loop.first %}
             {% assert f.data_type.alignment_requirement <= t.inner_type.alignment_requirement %}
         {% else %}
-    {{ _pad_to_alignment(f.data_type.alignment_requirement)|trim|remove_blank_lines }}
+    {{ _pad_to_alignment(f.data_type.alignment_requirement)|trim }}
         {% endif %}
     {   // {{ f }}
-        {{ _serialize_any(f.data_type, 'obj->' + (f|id), offset)|trim|remove_blank_lines|indent }}
+        {{ _serialize_any(f.data_type, 'obj->' + (f|id), offset)|trim|indent }}
     }
     {% endfor %}
 {% elif t.inner_type is UnionType %}
     {   // Union tag field: {{ t.inner_type.tag_field_type }}
         {{
             _serialize_integer(t.inner_type.tag_field_type, 'obj->_tag_', 0|bit_length_set)
-           |trim|remove_blank_lines|indent
+           |trim|indent
         }}
     }
     {% for f, offset in t.inner_type.iterate_fields_with_offsets() %}
     {{ 'if' if loop.first else 'else if' }} ({{ loop.index0 }}U == obj->_tag_)  // {{ f }}
     {
         {%- assert f.data_type.alignment_requirement <= (offset.min) %}
-        {{ _serialize_any(f.data_type, 'obj->' + (f|id), offset)|trim|remove_blank_lines|indent }}
+        {{ _serialize_any(f.data_type, 'obj->' + (f|id), offset)|trim|indent }}
     }
     {%- endfor %}
     else
@@ -72,7 +72,7 @@
     }
 {% else %}{% assert False %}
 {% endif %}
-    {{ _pad_to_alignment(t.inner_type.alignment_requirement)|trim|remove_blank_lines }}
+    {{ _pad_to_alignment(t.inner_type.alignment_requirement)|trim }}
     // It is assumed that we know the exact type of the serialized entity, hence we expect the size to match.
 {% if not t.inner_type.bit_length_set.fixed_length %}
     {{ assert('offset_bits >= %sULL'|format(t.inner_type.bit_length_set.min)) }}

diff --git a/src/nunavut/lang/cpp/support/variable_length_array.hpp b/src/nunavut/lang/cpp/support/variable_length_array.hpp
@@ -1415,8 +1415,13 @@ class VariableLengthArray<bool, MaxSize, Allocator>
         {
             if (new_data != data_)
             {
+                std::fill_n(new_data, bits2bytes(no_shrink_capacity), 0);
                 move_and_free(new_data, data_, size_, capacity_, alloc_);
-            }  // else the allocator was able to simply extend the reserved area for the same memory pointer.
+            }
+            else  // the allocator was able to simply extend the reserved area for the same memory pointer.
+            {
+                std::fill_n(data_ + bits2bytes(capacity_), (no_shrink_capacity - capacity_) / 8U, 0);
+            }
             data_ = new_data;
             capacity_ = no_shrink_capacity;
         }
@@ -1466,6 +1471,7 @@ class VariableLengthArray<bool, MaxSize, Allocator>
         }
         if (minimized_data != data_)
         {
+            std::fill_n(minimized_data, bits2bytes(size_), 0);  // Avoid uninitialized reads during modification
             move_and_free(minimized_data, data_, size_, capacity_, alloc_);
         }  // else the allocator was able to simply shrink the reserved area for the same memory pointer.
         data_     = minimized_data;