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 convenience error handling macros #421

Merged
merged 3 commits into from
Apr 11, 2023

Conversation

methylDragon
Copy link
Contributor

@methylDragon methylDragon commented Apr 9, 2023

Needed for: ros2/ros2#1405

This adds a bunch of macros to append error strings.

So before you would do:

    auto old_error = rcutils_get_error_msg();
    rcutils_reset_error();
    RCUTILS_SET_ERROR_MSG_WITH_FORMAT_STRING("New error:\n%s", old_error.str);

Now you can do:

    RCUTILS_SET_ERROR_MSG_AND_APPEND_PREV_ERROR("New error");

(And similarly for setting error with format strings, and also safe fprint)

If no old error was set, then the behavior is just like RCUTILS_SET_ERROR_MSG

@methylDragon methylDragon force-pushed the runtime_interface_reflection_allocators branch from 1f750fa to fed5d64 Compare April 9, 2023 13:12
include/rcutils/error_handling.h Outdated Show resolved Hide resolved
include/rcutils/error_handling.h Outdated Show resolved Hide resolved
include/rcutils/error_handling.h Outdated Show resolved Hide resolved
include/rcutils/error_handling.h Outdated Show resolved Hide resolved
include/rcutils/error_handling.h Outdated Show resolved Hide resolved
include/rcutils/error_handling.h Outdated Show resolved Hide resolved
Signed-off-by: methylDragon <[email protected]>
@clalancette
Copy link
Contributor

Before going too far forward with this, I think we should talk about the error handling in our C layers in a more general sense. Mich filed #269 3 years ago to consider this issue. I think we should come up with a general strategy before we introduce new code here. Maybe we can talk about it at the ROS 2 weekly tomorrow?

@methylDragon
Copy link
Contributor Author

Before going too far forward with this, I think we should talk about the error handling in our C layers in a more general sense. Mich filed #269 3 years ago to consider this issue. I think we should come up with a general strategy before we introduce new code here. Maybe we can talk about it at the ROS 2 weekly tomorrow?

@clalancette

I think we can do that, but some of my other PRs pending the rmw freeze are using code from this PR...

I was thinking that since these are slight extensions to the existing macros, it's not too much of an issue to merge them in first, then consider a more general strategy after the freeze?

@clalancette
Copy link
Contributor

I think we can do that, but some of my other PRs pending the rmw freeze are using code from this PR...

So I thought that everything remaining to be merged was "bonus", no? If that's the case, then we can leave them out of Iron and target them for J-Turtle. If that's not the case, can you point to what still needs to get in?

@wjwwood
Copy link
Member

wjwwood commented Apr 10, 2023

If that's not the case, can you point to what still needs to get in?

I think it's the prs attached to ros2/ros2#1405. I think we should try to get them in. If this pr doesn't get in, I think the others can still go in, he'll just have to expand the macros in this pr manually. Which honestly is what is done in the code base today (there are lots of places where we do what these macros do already).

@wjwwood
Copy link
Member

wjwwood commented Apr 10, 2023

Mich filed #269 3 years ago to consider this issue. I think we should come up with a general strategy before we introduce new code here.

For what it's worth, the code that these new macros do is already present in the code base, and was my suggestion for what should be done right now: #269 (comment)

Example of it already in the code base:

https://github.com/ros2/rcl/blob/67919d56192f12c96147bcc1d459f9aeea880b3f/rcl/src/rcl/arguments.c#L341-L345

I think #269 is more about a better way to do this stuff, which I think is a worthwhile thing to discuss, but what's in this pr is just making macros to de-duplicate existing code, at least in my opinion.

@methylDragon methylDragon merged commit e5cce95 into rolling Apr 11, 2023
@delete-merged-branch delete-merged-branch bot deleted the runtime_interface_reflection_allocators branch April 11, 2023 06:02
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.

3 participants