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

Use the new rcutils_strerror function. #239

Merged
merged 3 commits into from Feb 28, 2020
Merged

Use the new rcutils_strerror function. #239

merged 3 commits into from Feb 28, 2020

Conversation

clalancette
Copy link
Contributor

Signed-off-by: Chris Lalancette clalancette@openrobotics.org

Part of ros2/rcutils#196

throw std::runtime_error(errmsg);
#else
throw std::runtime_error(strerror(errno));
#endif // _WIN32
Copy link
Member

Choose a reason for hiding this comment

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

Instead, replace all of this with rcutils_snprintf()?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For consistencies sake, I did switch to rcutils_snprintf. As far as I can tell, this doesn't remove the need for the rest of the error checking, so I left the rest as-is.

Copy link
Member

Choose a reason for hiding this comment

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

Oh maybe I meant to say format string, it does the whole “how big? Allocate, snprintf, error handling” thing:

https://github.com/ros2/rcutils/blob/bd590e5253672957e8c7e070523fa725a4187db2/src/format_string.c#L32

Though it would use an rcutils allocator instead.

Idiomatic c++ would use streams instead... or the format stuff if we had c++20.

Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>
Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>
Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>
@clalancette
Copy link
Contributor Author

This is the last of the series that doesn't have an approval. Once this one is approved, I think I'm ready to merge this. @wjwwood any further thoughts about this one?

@dirk-thomas
Copy link
Member

It would be good to update the title of this PR (as well as siblings) to reflect the current state: using rcutils_strerror.

@clalancette clalancette changed the title Use the new rcutils_safe_strerror function. Use the new rcutils_strerror function. Feb 26, 2020
Copy link
Member

@wjwwood wjwwood left a comment

Choose a reason for hiding this comment

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

I had one follow up comment but it is t blocking.

@clalancette clalancette merged commit 69ee858 into ros2 Feb 28, 2020
@clalancette clalancette deleted the log_to_stderr branch February 28, 2020 20:22
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.

None yet

3 participants