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.
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Improve wide string (de)serialization #113
Improve wide string (de)serialization #113
Changes from 7 commits
60fba6f
2ee730a
69cbd23
e37b4d3
349d1c9
9a9776b
c4eb425
f9a8358
7f2533a
3ba0207
090b784
e8dba93
24099c5
945f193
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there any particular reason to use
uint32_t
here? I know thatrosidl_runtime_c__U16String
usesuint_least16_t
for the data, but on all platforms we support we have a true uint16_t. So it seems to me that we could change this to:There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The main reason is interoperability / backward compatibility. The serialization of the
wstring
is using 4 bytes per character.I initially did what you suggest, and used a
uint16_t
, but changed it when the tests intest_communication
failed betweenrmw_cyclonedds
andrmw_fastrtps
forWStrings
type.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note: we can change this in the future if we don't mind breaking backward compatibility, but should do it at the same time in all DDS-based RMW implementations
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, got it. OK, interoperability is a really good reason.
In that case, I'm totally fine with leaving this as-is, but please put in a comment saying that we are using uint32_t for the interoperability reason.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
f9a8358