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

Review the contract of get_typesupport_handle_function #100

Open
clalancette opened this issue Jan 5, 2021 · 0 comments · May be fixed by #102
Open

Review the contract of get_typesupport_handle_function #100

clalancette opened this issue Jan 5, 2021 · 0 comments · May be fixed by #102
Labels

Comments

@clalancette
Copy link
Contributor

While working on ros2/rmw_cyclonedds#274 and ros2/rmw#293 , a review of the contract of get_typesupport_handle_function revealed it was less than ideal.

In particular, get_typesupport_handle_function is used to probe for the existence of typesupport libraries. If a particular implementation doesn't support that typesupport, then an rcutils error is set and nullptr is returned. If the implementation does support that typesupport, but setting it up failed, then an rcutils error is set and nullptr is returned. Finally, if the implementation does support that typesupport, and setting it up succeeded, then no rcutils error is set and a pointer is returned.

Setting an rcutils error when a typesupport doesn't exist seems to somewhat defeat the purpose of it being a "probe". It leads to code like that in ros2/rmw_cyclonedds#274 and ros2/rmw_cyclonedds#271, which works, but is kind of ugly.

My proposal here is that we change the contract of get_typesupport_handle_function so that if the implementation doesn't support that typesupport, then no rcutils error is set and nullptr is returned. That way, the caller can disambiguate between no support (nullptr + no rcutils error), a fatal error in setting up (nullptr + rcutils error), and success (real pointer). This change will require us to review all uses of get_typesupport_handle_function to make sure that the callers are prepared for this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant