-
Notifications
You must be signed in to change notification settings - Fork 412
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
Fix documentation of RCLCPP_[INFO,WARN,...]
#1943
Conversation
@@ -114,7 +114,6 @@ def get_rclcpp_suffix_from_features(features): | |||
@[ end for]@ | |||
@[ if 'stream' not in feature_combination]@ | |||
* \param ... The format string, followed by the variable arguments for the format string. | |||
* It also accepts a single argument of type std::string. |
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 thought about adding a small remark something like this (but I am not sure if it is the best place here):
* \param ... The format string (should be a string literal), followed by the variable arguments for the format string.
* If you want to print dynamic strings, i.e. a std::string, please use the _STREAM variant instead.
Using a runtime/dynamic string in this printf
style context is almost always a bad idea and the _STREAM
variant would be perfectly fine for this task.
I think this also completely resolves the problem from the above mentioned PR, as using the _STREAM
variant should have no downsides and has the benefit of being safe.
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.
Using a runtime/dynamic string in this
printf
style context is almost always a bad idea and the_STREAM
variant would be perfectly fine for this task.
I disagree with this assessment (so I'm glad it is not in this PR :). In particular, there are situations where it is helpful to build up a string at runtime that you then both want to output to the log (via RCLCPP_WARN
or whatever), and then also use for another purpose (like as a return value). While I guess you can use STREAM for that, I also don't see much of a problem with:
std::string do_something()
{
int ret = function_call_that_failed();
if (ret < 0) {
std::string my_error_return = "Failed to call function: " + std::to_string(ret);
RCLCPP_WARN(get_logger(), "%s", my_error_return.c_str());
}
return my_error_return;
}
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.
Sorry my messaging was probably not so clear. What you have in your example is perfectly fine.
What I am afraid of is people doing is this:
std::string msg = "some error message from elsewhere";
RCLCPP_INFO(logger, ("some suffix: " + msg).c_str());
(this example is from #570)
In the companies I worked at the unqualified macros where only used if there is no interpolation at all and otherwise my colleagues always used the _STREAM
variant, which gave me the impression that this is the "typical" behavior.
In this case I thought using the _STREAM
macro would be much more intuitive and especially steer away from the example above (which I know unfortunately have already seen in our codebase).
Your example would also work with the _STREAM
:
std::string do_something()
{
int ret = function_call_that_failed();
if (ret < 0) {
std::string my_error_return = "Failed to call function: " + std::to_string(ret);
RCLCPP_WARN_STREAM(get_logger(), my_error_return);
}
return my_error_return;
}
I am also perfectly fine with just removing the line, so that we can close this PR.
@ros-pull-request-builder retest this please |
I just realized this was targeting humble. I'm going to retarget this to the master branch, and we can backport it to |
In the pull request ros2#1442 the ability to use `std::string` as the first argument to the logging functions was (rightfully) removed. This commit just corrects the documentation of the macro, since it confused me a bit ;) Signed-off-by: Daniel Reuter <daniel.robin.reuter@googlemail.com>
91ff185
to
64b050d
Compare
All of the yellow CI is unrelated to this PR, so I'm going to merge this. Thanks for the contribution. |
In the pull request #1442 the ability
to use
std::string
as the first argument to the logging functions was(rightfully) removed.
This commit just corrects the documentation of the macro, since it
confused me a bit ;)