Skip to content
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

Implement copy function for C messages #650

Merged
merged 2 commits into from
Jan 25, 2022
Merged

Conversation

hidmic
Copy link
Contributor

@hidmic hidmic commented Jan 20, 2022

Precisely what the title says. While C++ messages overload the assignment operator, plain assignment of C messages results in shallow copy of pointers which in turn leads to ownership issues (e.g. double free crashes) down the line. This patch amends this.

Depends on #648.

@hidmic hidmic changed the title Implement copy function for C messages. [DO NOT REVIEW] Implement copy function for C messages. Jan 20, 2022
@hidmic hidmic changed the title [DO NOT REVIEW] Implement copy function for C messages. [DO NOT REVIEW] Implement copy function for C messages Jan 20, 2022
Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
@hidmic hidmic changed the title [DO NOT REVIEW] Implement copy function for C messages Implement copy function for C messages Jan 21, 2022
@hidmic
Copy link
Contributor Author

hidmic commented Jan 21, 2022

CI up to rosidl_generator_c and rosidl_runtime_c (nothing uses this API just yet):

  • Linux Build Status
  • Linux-aarch64 Build Status
  • Windows Build Status

@hidmic
Copy link
Contributor Author

hidmic commented Jan 21, 2022

@gbiggs @j-rivero PTAL!

* This functions performs a deep copy, as opposed to the shallow copy that
* plain assignment yields.
*
* \param[out] input a pointer to a rosidl_runtime_c__String structure
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
* \param[out] input a pointer to a rosidl_runtime_c__String structure
* \param[in] input a pointer to a rosidl_runtime_c__String structure

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in ab79f66.

Copy link
Contributor

@j-rivero j-rivero left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some minor comments, otherwise looks good to me. Great work.

Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
@hidmic hidmic requested a review from j-rivero January 25, 2022 13:24
@hidmic hidmic merged commit 4dd9883 into master Jan 25, 2022
@delete-merged-branch delete-merged-branch bot deleted the hidmic/copy-for-c-messages branch January 25, 2022 14:05
nnmm pushed a commit to nnmm/rosidl that referenced this pull request Mar 8, 2022
Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
nnmm pushed a commit to nnmm/rosidl that referenced this pull request Mar 8, 2022
Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
hidmic added a commit that referenced this pull request Apr 25, 2022
* Implement equality operator function for C messages. (#648)

Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>

* Implement copy function for C messages (#650)

Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>

* Replace rcutils_allocator with the system allocator

Signed-off-by: Nikolai Morin <nnmmgit@gmail.com>

* Set the output size unconditionally when copying sequences

Signed-off-by: Nikolai Morin <nnmmgit@gmail.com>

Co-authored-by: Michel Hidalgo <michel@ekumenlabs.com>
hidmic added a commit that referenced this pull request Apr 25, 2022
* Implement equality operator function for C messages. (#648)

Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>

* Implement copy function for C messages (#650)

Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>

* Replace rcutils_allocator with the system allocator

Signed-off-by: Nikolai Morin <nnmmgit@gmail.com>

* Set the output size unconditionally when copying sequences

Signed-off-by: Nikolai Morin <nnmmgit@gmail.com>

Co-authored-by: Michel Hidalgo <michel@ekumenlabs.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants