Remove rethrow in extern c code #82
Conversation
|
Testing
|
Signed-off-by: Stephen Brawner <brawner@gmail.com> squash! Remove rethrow in extern c code
brawner
force-pushed
the
brawner/catch-exceptions-extern-c
branch
from
8cf53ce to
a18922c
Compare
|
Err, fprintf requires
Builds with fault injection:
|
|
@ros-pull-request-builder retest this please (using custom job config from ros-infrastructure/ros_buildfarm#828) |
|
Windows build warning on fault injection jobs is unrelated to this PR (rcl_action). |
| throw std::runtime_error( | ||
| "Could not load library " + library_path + ": " + | ||
| std::string(e.what())); | ||
| fprintf(stderr, "Could not load library %s: %s\n", library_path.c_str(), e.what()); |
There was a problem hiding this comment.
Would it be better to set the error message with something like RCUTILS_SET_ERROR_MSG?
There was a problem hiding this comment.
I think that makes sense. It does add a direct dependency to rcutils, but it already indirectly depended on rcutils anyway. I converted these fprintf statements, converted one previously existing one and added an error message for the other nullptr case. This causes the API to diverge a small amount from the rosidl_typesupport_cpp version, so if you like this change I can create a similar PR for that package.
There was a problem hiding this comment.
This causes the API to diverge a small amount from the rosidl_typesupport_cpp version
Since the C++ API can leverage exceptions for this kind of error reporting I would think it could stay as it is.
Does any existing code need to be updated to check / reset the error message?
There was a problem hiding this comment.
I'm not that familiar with the use of this package, so you might have to give me a couple of pointers. The only location this function is used in this package is in the .em files. (https://github.com/ros2/rosidl_typesupport/blob/master/rosidl_typesupport_c/resource/msg__type_support.cpp.em#L98), but I'm not sure where that function is called in ROS 2 as I can only find other instances inside .em files.
Is there something better I can be searching for to answer that question?
There was a problem hiding this comment.
I don't have any specific location in mind. Just searching for code which calls the API would be the only suggestion I have.
There was a problem hiding this comment.
I looked through the code, searching for:
rosidl_typesupport_c__get_message_typesupport_handle_functionROSIDL_TYPESUPPORT_INTERFACE__MESSAGE_SYMBOL_NAMEROSIDL_GET_MSG_TYPE_SUPPORTROSIDL_GET_SRV_TYPE_SUPPORT
The first two are ultimately just used in the last two macros as far as I can tell. The last two are used extensively, but just in test files where they are checked for a nullptr and expected to succeed. If they don't, generally the test fails immediately, but there might be possibilities that the error message gets overwritten if the nullptr is passed to a subsequent function.
It's possible I may be missing something, but I'll be happy to address issues in followup PRs if they come up.
Signed-off-by: Stephen Brawner <brawner@gmail.com>
* Remove rethrow in extern c code Signed-off-by: Stephen Brawner <brawner@gmail.com> squash! Remove rethrow in extern c code * Convert fprintf to rcutils error msg Signed-off-by: Stephen Brawner <brawner@gmail.com>
* Remove rethrow in extern c code Signed-off-by: Stephen Brawner <brawner@gmail.com> squash! Remove rethrow in extern c code * Convert fprintf to rcutils error msg Signed-off-by: Stephen Brawner <brawner@gmail.com>
rosidl_typesupport_ccode had some rethrown exceptions which were inside extern c functions. I believe, but not entirely sure, this is not recommended since there is no way for the C code to handle them and they would otherwise be fatal errors. MSVC's optimizer was also treating this code strangely when I tried to catch the exception from a c++ test and would get different results by adding unrelated changes.Example failure with fault injection test:
Signed-off-by: Stephen Brawner brawner@gmail.com