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

Simplify the various 'is_valid' calls #1086

Draft
wants to merge 9 commits into
base: rolling
Choose a base branch
from

Conversation

clalancette
Copy link
Contributor

Currently if you call one of the rcl "is_valid" functions on an uninitialized pointer of the correct type, it will return false but it will also set an error. But this doesn't seem correct; just asking the question of whether it is valid should never return an error. This PR fixes that.

Note that if you pass a NULL pointer into the "is_valid" function, you'll still get an error set. That seems reasonable since you are basically asking a nonsense question.

Users should be able to query whether a subscription
is valid without having an error set.  Only set an
error if an invalid pointer (e.g. NULL) was passed in.

Signed-off-by: Chris Lalancette <[email protected]>
A user should be able to query whether a client is
valid without an error being set.  Fix that here.

Signed-off-by: Chris Lalancette <[email protected]>
A user should be able to query whether an event is
valid without an error being set.  Fix that here.

Signed-off-by: Chris Lalancette <[email protected]>
A user should be able to query whether a node is
valid without setting an error.  Fix that here.

Signed-off-by: Chris Lalancette <[email protected]>
A user should be able to query whether a publisher is
valid without it setting an error.  Fix that here.

Signed-off-by: Chris Lalancette <[email protected]>
A user should be able to query whether a service is
valid without it setting an error.  Fix that here.

Signed-off-by: Chris Lalancette <[email protected]>
A user should be able to query whether a service event
publisher is valid without it setting an error.  Fix
that here.

Signed-off-by: Chris Lalancette <[email protected]>
A user should be able to query whether a wait_set is
valid without it setting an error, which it already does.
However, passing a NULL into it is logically invalid;
we can't possibly answer that question.  So set an error
in that case.

Signed-off-by: Chris Lalancette <[email protected]>
@clalancette
Copy link
Contributor Author

CI:

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

Signed-off-by: Chris Lalancette <[email protected]>
Copy link
Collaborator

@fujitatomoya fujitatomoya left a comment

Choose a reason for hiding this comment

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

lgtm

just asking the question of whether it is valid should never return an error.

makes sense to me.

@clalancette
Copy link
Contributor Author

The more I look at this, the less I like it.

If you are looking at these is_valid calls as part of the external rcl API, then I think this change makes sense. However, the vast majority of calls to is_valid comes from within rcl itself. And in that case, it becomes difficult to determine the split of responsibilities between the is_valid function itself, and the caller, because sometimes the caller needs to set the error, and sometimes the caller needs to reset the error and set something else.

I'm going to turn this into a draft for now until I come up with something better.

@clalancette clalancette marked this pull request as draft August 9, 2023 12:28
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