C++: Avoid name collisions and add missing const begin()/end() methods to VLA #287
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
pavel-kirienko
requested review from
thirtytwobits
and removed request for
thirtytwobits
pavel-kirienko
requested review from
asmfreak and
thirtytwobits
and removed request for
asmfreak
|
Kudos, SonarCloud Quality Gate passed! |
|
We already have IndexOf so here I am trying to follow the same convention.
Do you think TypeOfAttribute is a better name? "Service" does not seem
related.
…On Fri, Mar 3, 2023, 22:27 Scott Dixon ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In src/nunavut/lang/cpp/templates/_composite_type.j2
<#287 (comment)>:
> + /// 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.
+ static constexpr {{ typename_unsigned_length }} ExtentBytes = {# -#}
+ {{ composite_type.extent // 8 }}UL;
+ static constexpr {{ typename_unsigned_length }} SerializationBufferSizeBytes = {# -#}
+ {{ composite_type.inner_type.extent // 8 }}UL;
+ static_assert(ExtentBytes >= SerializationBufferSizeBytes, "Internal constraint violation");
+ static_assert({# -#}
+ ExtentBytes < (std::numeric_limits<{{ typename_unsigned_bit_length }}>::max() / 8U), {# -#}
+ "This message is too large to be handled by the selected types");
+{%- for field in composite_type.fields_except_padding %}
+ {%- if loop.first %}
+ struct TypeOf
TypeOf needs docs. Also, this is a really generic name that suggest some
greater type system. Perhaps call it TypeOfService such that you get
TypeOfService::Response which is a nice, English sentence "The type of
the service response." etc.
—
Reply to this email directly, view it on GitHub
<#287 (review)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAZFIZEHQNKT7ONHULLMJSLW2JH4PANCNFSM6AAAAAAVOZ45KU>
.
You are receiving this because you authored the thread.Message ID:
***@***.***>
|
thirtytwobits
approved these changes
Mar 3, 2023
pavel-kirienko
added a commit
that referenced
this pull request
Mar 5, 2023
…or mistake introduced in the prior PR (#290) - Do not lose blank lines in doc comments. - Move `Request`/`Response` aliases out of the `_traits_`; this was changed by mistake in #287 - Repair the vscode run/debug configuration The naming of the service pseudo-type `Service_{{major}}_{{minor}}` non-obvious and I would like to suggest renaming it such that the service pseudo-type is named after the service itself suffixed with the version numbers. This would be in line with the way it is implemented in Python. Instead of this: ```c++ uavcan::_register::Access::Request_1_0 uavcan::_register::Access::Response_1_0 uavcan::_register::Access::Service_1_0 ``` We now have this: ```c++ uavcan::_register::Access::Request_1_0 uavcan::_register::Access::Response_1_0 uavcan::_register::Access_1_0 ``` Then there already are aliases inside `Access_1_0` which means that the full set of the available names is as follows: ```c++ uavcan::_register::Access::Request_1_0 uavcan::_register::Access::Response_1_0 uavcan::_register::Access_1_0 uavcan::_register::Access_1_0::Request uavcan::_register::Access_1_0::Response ``` See related discussion in #155 and #190
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #286
In C++,
serializeanddeserializeare now free-standing functions. The alternative would be to name them_serialize_and_deserialize_, but I imagine this to be bad form.One unused error tag
SERIALIZATION_INVALID_ARGUMENThas been removed. The others have been renamed into CamelCase for consistency, as ALL_CAPS is reserved for macros.I also added
_traits_::TypeOffor introspection; it is often helpful in user code as the alternative requires the use ofdecltype:We may also introduce a wrapper like
nunavut::traits<T>if direct usage of_traits_is considered hard to read.