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

change throw_from_rcl_error to throw from_rcl_error #647

Closed
sloretz opened this issue Mar 4, 2019 · 5 comments
Closed

change throw_from_rcl_error to throw from_rcl_error #647

sloretz opened this issue Mar 4, 2019 · 5 comments
Labels
enhancement New feature or request

Comments

@sloretz
Copy link
Contributor

sloretz commented Mar 4, 2019

Feature request

Currently rclcpp has a function to throw an exception based on an rcl error.

rclcpp::exceptions::throw_from_rcl_error(ret, "Failed to add time jump callback");

This is a request to change that function to return an exception without throwing.

throw rclcpp::exceptions::from_rcl_error(ret, "Failed to add time jump callback");

I think this could help static analysis tools like cppcheck recognize when code won't be executed due to an exception. This would remove the need for #646

Implementation considerations

throw_from_rcl_error could be refactored to use from_rcl_error rather than changing all the code at once.

@sloretz sloretz added the enhancement New feature or request label Mar 4, 2019
@sloretz sloretz changed the title change throw_from_rcl_error to exception_from_rcl_error change throw_from_rcl_error to throw from_rcl_error Mar 4, 2019
@wjwwood
Copy link
Member

wjwwood commented Mar 5, 2019

Seems reasonable, at the very least to factor the creation of the exception and the throw into separate functions.

@jhdcs
Copy link
Contributor

jhdcs commented Mar 26, 2019

Mind if I give this a go? I was thinking of renaming "throw_from_rcl_error" to either "from_rcl_error" or "gen_rcl_exception", then refactoring everything that called the original function to "throw <from_rcl_error>". Unless, of course, you want to do something besides throwing at any of the locations.

Also, out of curiosity, is there a reason we aren't using enums for return codes? And should I put that in a separate question?

@wjwwood
Copy link
Member

wjwwood commented Mar 26, 2019

Mind if I give this a go?

Sure, please have a go at it.

I was thinking of renaming "throw_from_rcl_error" to either "from_rcl_error" or "gen_rcl_exception", then refactoring everything that called the original function to "throw <from_rcl_error>". Unless, of course, you want to do something besides throwing at any of the locations.

I think from_rcl_error is better than gen_rcl_exception myself, but exception_from_rcl_error would be another alternative that I think. Though in context, throw rclcpp::exceptions::from_rcl_error is pretty clear already. On the other hand, if the user does using rclcpp::exceptions::from_rcl_error and later does throw from_rcl_error(...) it's a bit less clear and throw exception_from_rcl_error(...) would be clearer in that case.

I don't mind which you do, but the already proposed ones seem like the best name I've heard so far.

Also, consider the line from the description:

throw_from_rcl_error could be refactored to use from_rcl_error rather than changing all the code at once.

Avoiding touching all the existing code would be a nice thing to have, while still enabling users to generate the exception without throwing it in cases where that's needed.


Also, out of curiosity, is there a reason we aren't using enums for return codes? And should I put that in a separate question?

I think it's mostly a historical reason, but avoiding enums in C (and using #define instead) forces us to pick a value for each constant, whereas enums would let us list them and automatically get values from their order, but then later we'd be encouraged to reorder them or inject new ones between existing ones, causing the values to change. By using the defines we have to be explicit about the values, and plan for future ones, reducing the likelihood that we need to change the value of an existing return code.

@sloretz
Copy link
Contributor Author

sloretz commented Apr 2, 2019

I think this ticket is invalid as written. I assumed from_rcl_error could return a base exception type, but I didn't realize that only triggers the copy constructor of the base type. The derived exception type would be lost and catch (DerivedType &) would never trigger.

The original issue is having to work around cppcheck's false positive. I think a better solution would would be for cppcheck to understand [[noreturn]] and then to mark throw_from_rcl_error with that.

Another option would be to add a cppcheck library config file for rclcpp that has <noreturn> set for throw_from_rcl_error, but that requires changes to ament_cppcheck and ament_cmake_cppcheck. They would have to take config files, and then downstream libraries would need to be able to get cppcheck config files from upstream ones. That seems like more effort than occasional PRs to work around false positives.

@sloretz
Copy link
Contributor Author

sloretz commented Apr 16, 2019

Closing now that #678 is merged

@sloretz sloretz closed this as completed Apr 16, 2019
@sloretz sloretz removed the backlog label Apr 16, 2019
nnmm pushed a commit to ApexAI/rclcpp that referenced this issue Jul 9, 2022
Signed-off-by: Stephen Brawner <brawner@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

4 participants