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

[dreuter]

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

[dreuter]

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.

[clalancette]

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

[dreuter]

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.

[clalancette]

@ros-pull-request-builder retest this please

[clalancette]

I just realized this was targeting humble. I'm going to retarget this to the master branch, and we can backport it to humble once we get it in.

[clalancette]

CI:

• Linux
• Linux-aarch64
• Windows

[clalancette]

Another CI after I rebased:

• Linux
• Linux-aarch64
• Windows

[clalancette]

All of the yellow CI is unrelated to this PR, so I'm going to merge this. Thanks for the contribution.