Skip to content

Make rclrs panic-free as far as possible #190

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

Merged
merged 1 commit into from
Jun 6, 2022
Merged

Make rclrs panic-free as far as possible #190

merged 1 commit into from
Jun 6, 2022

Conversation

nnmm
Copy link
Contributor

@nnmm nnmm commented Jun 4, 2022

This is achieved by making CString errors a part of RclrsError. This was made possible by #183, which made the error struct an enum.

Example error output:

Error: Could not convert string 'minimal_subscriber' to CString

Caused by:
   nul byte found in provided data at position: 13

The nul byte is not rendered in the output, as you can see.

This is achieved by making CString errors a part of RclrsError, since RclrsError is now an enum.
@nnmm
Copy link
Contributor Author

nnmm commented Jun 4, 2022

Note for reviewers: This does not increase the size of the RclrsError struct, since the other variants are larger.

The map_err() is of course a little verbose, but users should never need to do CString conversions anyway, so the API is clean. But still, if it's too ugly, we could remove the string member from this enum variant and impl From<NulError> for RclrsError.

@nnmm nnmm requested review from esteve and jhdcs June 4, 2022 12:26
Copy link
Collaborator

@jhdcs jhdcs left a comment

Choose a reason for hiding this comment

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

These look to be fairly simple and reasonable changes to me.

@nnmm
Copy link
Contributor Author

nnmm commented Jun 6, 2022

Thanks @jhdcs!

@nnmm nnmm merged commit 318bebf into master Jun 6, 2022
@delete-merged-branch delete-merged-branch bot deleted the nul_error branch June 6, 2022 19:47
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