-
Notifications
You must be signed in to change notification settings - Fork 67
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
Update serialization/deserialization API documentation. #258
Changes from 2 commits
cca21b7
b821e77
6e652f2
2625c33
b43d14e
106d36c
839edab
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||||||
---|---|---|---|---|---|---|---|---|---|---|
|
@@ -29,9 +29,49 @@ extern "C" | |||||||||
* more complex and is therefore aliased. | ||||||||||
*/ | ||||||||||
typedef rcutils_uint8_array_t rmw_serialized_message_t; | ||||||||||
|
||||||||||
/// Return a zero initialized serialized message struct. | ||||||||||
#define rmw_get_zero_initialized_serialized_message rcutils_get_zero_initialized_uint8_array | ||||||||||
|
||||||||||
/// Initialize a serialized message, zero initializing its contents. | ||||||||||
/** | ||||||||||
* \param[inout] serialized_message a pointer to the serialized message to initialize | ||||||||||
* \param[in] message_capacity the size of the memory to allocate | ||||||||||
* \param[in] allocator the allocator to use for the memory allocation | ||||||||||
* \return `RMW_RET_OK` if successful, or | ||||||||||
* \return `RMW_RET_INVALID_ARGUMENT` if any arguments are invalid, or | ||||||||||
* \return 'RMW_RET_BAD_ALLOC` if no memory could be allocated correctly | ||||||||||
* \return `RMW_RET_ERROR` if an unexpected error occurs | ||||||||||
*/ | ||||||||||
#define rmw_serialized_message_init rcutils_uint8_array_init | ||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Just a thought here. I would find this documentation easier to read if
The same goes for all of the other |
||||||||||
|
||||||||||
/// Finalize a serialized message. | ||||||||||
/** | ||||||||||
* Passing an uninitialized instance to this function leads to undefined | ||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is it better to make this a checked pre-condition? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. At the very least, it's probably a good idea to add a There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This needs an update. Passing uninitialized arguments will always be UB, and known states (zero-initialized or initialized) will be handled correctly in runtime. |
||||||||||
* behavior. | ||||||||||
* | ||||||||||
* \param[in] serialized_message pointer to the serialized message to be cleaned up | ||||||||||
* \return `RMW_RET_OK` if successful, or | ||||||||||
* \return `RMW_RET_INVALID_ARGUMENT` if the serialized_message argument is invalid | ||||||||||
* \return `RMW_RET_ERROR` if an unexpected error occurs | ||||||||||
*/ | ||||||||||
#define rmw_serialized_message_fini rcutils_uint8_array_fini | ||||||||||
|
||||||||||
/// Resize the internal buffer of the serialized message. | ||||||||||
/** | ||||||||||
* The internal buffer of the serialized message can be resized dynamically if needed. | ||||||||||
* If the new size is smaller than the current capacity, then the memory is | ||||||||||
* truncated. | ||||||||||
* Be aware, that this might deallocate the memory and therefore invalidates any | ||||||||||
* pointers to this storage. | ||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This sentence is grammatically correct, but I would still find a modification slightly easier to read:
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sounds good. At some point we should correct |
||||||||||
* | ||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should we add There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We can constrain possible states to zero-initialized or initialized, but IMO to say that using an uninitialized variable is UB goes without saying. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The thing here is that we are not just saying "it's undefined", but we are in a way specifying whether implementers of this function declaration need to add a check for null pointers, etc. That, I think, makes it worth being more explicit. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think the added |
||||||||||
* \param[inout] serialized_message pointer to the serialized message to be resized | ||||||||||
* \param[in] new_size the new size of the internal buffer | ||||||||||
* \return `RMW_RET_OK` if successful, or | ||||||||||
* \return `RMW_RET_INVALID_ARGUMENT` if new_size is set to zero | ||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||||||
* \return `RMW_RET_BAD_ALLOC` if memory allocation failed, or | ||||||||||
* \return `RMW_RET_ERROR` if an unexpected error occurs | ||||||||||
*/ | ||||||||||
#define rmw_serialized_message_resize rcutils_uint8_array_resize | ||||||||||
|
||||||||||
#if __cplusplus | ||||||||||
|
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.