-
Notifications
You must be signed in to change notification settings - Fork 101
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 RCUTILS_CAN_SET_ERROR_MSG_AND_RETURN_WITH_ERROR_OF() macro. #284
Conversation
To fault inject error messages as well as return codes. Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
include/rcutils/error_handling.h
Outdated
* | ||
* \param error_return_value the value returned as a result of a given error. | ||
*/ | ||
#define RCUTILS_CAN_SET_ERROR_MSG_AND_RETURN_WITH_ERROR_OF(error_return_value) \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some other ideas if you want to go with something a little shorter:
RCUTILS_CAN_SET_MSG_AND_RETURN_WITH_ERROR_OF(error_return_value);
RCUTILS_CAN_SET_AND_RETURN_ERROR_OF(error_return_value);
RCUTILS_CAN_SET_ERROR_MSG_AND_RETURN(error_return_value);
But also, should this macro take an error msg argument instead of building its own?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
RCUTILS_CAN_SET_MSG_AND_RETURN_WITH_ERROR_OF
Sounds good too.
But also, should this macro take an error msg argument instead of building its own?
I thought the same at first. Then it hit me that there's no reasonable error message a user can set to encompass all possible root causes for a given return code, nor it makes sense to duplicate all error messages set elsewhere in the function to stay consistent. We would end up with a bunch of user-defined dummy strings. That's why I made it generic.
Can you think of a use case for a meaningful error message?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's certainly a good point. The use would just be for debugging purposes. The more unique these messages are the easier it can be to track down where they come from. We can hold off until a strong demand arises.
@@ -223,6 +223,20 @@ rcutils_set_error_state(const char * error_string, const char * file, size_t lin | |||
} \ | |||
} while (0) | |||
|
|||
/// Indicate that the function intends to set an error message and return an error value. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would add some comment mentioning that this is part of the fault injection so people understand its intended purpose.
Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like you need to address an uncrustify issue. Once this is merged, I can create a release of rcutils.
Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
CI up to
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
To fault inject error messages as well as return codes. Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
To fault inject error messages as well as return codes. Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
To fault inject error messages as well as return codes.
Name is up for debate 😅