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

Add methods related to node options to NodeBuilder #186

Merged
merged 26 commits into from
Jun 10, 2022

Conversation

Soya-Onishi
Copy link
Contributor

@Soya-Onishi Soya-Onishi commented May 31, 2022

Closes #142

This PR add node options related to rcl_node_options_t.
However, methods related to allocator options (e.g. NodeBuilder::allocators.allocate) are not implemented because there are no way to return allocated heap pointer as far as I know.

@esteve esteve requested review from esteve, jhdcs and nnmm May 31, 2022 13:48
Copy link
Contributor

@nnmm nnmm left a comment

Choose a reason for hiding this comment

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

Thanks! I'll add more comments later.

Soya-Onishi and others added 3 commits June 3, 2022 00:02
Co-authored-by: Nikolai Morin <nnmmgit@gmail.com>
Co-authored-by: Nikolai Morin <nnmmgit@gmail.com>
@nnmm
Copy link
Contributor

nnmm commented Jun 2, 2022

@Soya-Onishi Great work! Could you also add Drop instances for rcl_node_options_t and rcl_arguments_t that call their fini functions?

@nnmm
Copy link
Contributor

nnmm commented Jun 3, 2022

I tested it out and got an error about rcl_arguments_fini being called twice (which makes sense, because rcl_node_options_fini also calls rcl_arguments_fini). So we should probably remove the Drop impl for rcl_arguments_t again, that would be easiest.

Apart from that, I only have one other comment (about enable_rosout), then it's ready to merge!

@nnmm
Copy link
Contributor

nnmm commented Jun 7, 2022

@Soya-Onishi Could you adjust the CString operations to match #190, which was merged in the meantime?

@nnmm
Copy link
Contributor

nnmm commented Jun 9, 2022

@Soya-Onishi I have another PR that conflicts with this one, #193. If you plan to make the couple of remaining fixes this week, I can wait with my PR until we merge this one, so you don't have extra work.

esteve
esteve previously approved these changes Jun 9, 2022
Copy link
Collaborator

@esteve esteve left a comment

Choose a reason for hiding this comment

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

@Soya-Onishi thanks for all the hard work and for the patience iterating with us! 🙂

@nnmm nnmm merged commit 30b1931 into ros2-rust:master Jun 10, 2022
@nnmm
Copy link
Contributor

nnmm commented Jun 10, 2022

Wonderful! Thanks @Soya-Onishi!

@Soya-Onishi
Copy link
Contributor Author

@nnmm @esteve
Thanks for your many advices and picks!

@nnmm
Copy link
Contributor

nnmm commented Jun 11, 2022

Feel free to join the Matrix chat by the way.

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.

Add support for node options
3 participants