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.

Sign up for GitHub

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

Jump to bottom

Fix documentation of RCLCPP_[INFO,WARN,...] #1943

Merged
merged 1 commit into from
Jun 7, 2022
Merged

Fix documentation of RCLCPP_[INFO,WARN,...] #1943

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 0 additions & 1 deletion rclcpp/resource/logging.hpp.em
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Copy link
Contributor Author

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.

Copy link
Contributor

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;
}

Copy link
Contributor Author

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.

@[ end if]@
*/
@{params = rclcpp_feature_combinations[feature_combination].params.keys()}@
Expand Down