ROS Resources: Documentation | Support | Discussion Forum | Index | Service Status | ros @ Robotics Stack Exchange
Ask Your Question
0

RCLCPP logging macros not a single expression

asked 2019-03-20 15:39:33 -0500

jdlangs gravatar image

updated 2019-03-20 16:00:07 -0500

I just discovered after a lot of confusion that the RCLCPP_ logging macros do not expand to a single expression and therefore cannot be used in brace-less if clauses and for loops. For example, this doesn't compile for me because the compiler sees an unexpected else:

if (true)
  RCLCPP_INFO(node->get_logger(), "true");
else
  RCLCPP_INFO(node->get_logger(), "false");

Is this expected?

Edit: added the mistakenly elided semi-colons from the original question

edit retag flag offensive close merge delete

Comments

"Expected" I don't know, but yes, all RCLCPP_* macros are two-liners: see here.

Might be an oversight: perhaps they should be wrapped in do { } while(0); constructs?

gvdhoorn gravatar image gvdhoorn  ( 2019-03-20 15:45:29 -0500 )edit

There was some discussion about this last fall here. I suspect that ;'s might help you in the above example, but, otherwise, ROS 2 linters require curly braces on all if statements, so it's understandable as to how this was not caught earlier.

allenh1 gravatar image allenh1  ( 2019-03-20 15:54:23 -0500 )edit

2 Answers

Sort by ยป oldest newest most voted
1

answered 2019-03-20 19:01:45 -0500

William gravatar image

@gvdhoorn is correct, it's an oversight that you cannot do:

if (condition)
  RCLCPP_INFO(...);
else
  RCLCPP_INFO(...);

The macro's contents should be wrapped within a do { ... } while(0). This is done in the equivalent C API's, see:

https://github.com/ros2/rcutils/blob/...

But the rclcpp one's do not do this:

https://github.com/ros2/rclcpp/blob/4...

I'd recommend opening an issue on rclcpp and ideally providing a pull request to help us fix it :D

edit flag offensive delete link more

Comments

Thanks, I was really confused that no one seemed to be bothered that function-call semantics had been broken, knowing that many more people would be falling into this non-obvious trap. I will be able to open a PR against rclcpp shortly.

jdlangs gravatar image jdlangs  ( 2019-03-21 08:52:16 -0500 )edit
0

answered 2019-03-20 15:48:53 -0500

marguedas gravatar image

updated 2019-03-20 17:54:09 -0500

This is expected. As of ROS Crystal, the logging macros expect a semi-colon ; at the end. The relevant issue can be found here

Edit: As gvdhoorn pointed out in his comment, the macros are two-liners. The ticket referenced above does mention that the chosen approach is a do / while requiring a semi-colon at the end. It looks like the implementation doesn't match.

edit flag offensive delete link more

Comments

1

And you should always use the curly braces to make sure that you're robust to the macro being compiled out.

tfoote gravatar image tfoote  ( 2019-03-20 15:52:19 -0500 )edit

Sorry for my sloppy question writing; my code does have the semi-colons after the macro calls and it still does not compile.

jdlangs gravatar image jdlangs  ( 2019-03-20 15:59:29 -0500 )edit
1

It's not semi-colons: it's curly braces that you are missing.

so single-line conditionals are not allowed.

if (..) {
}
else {
}
gvdhoorn gravatar image gvdhoorn  ( 2019-03-20 16:06:56 -0500 )edit

I don't follow why braces should be required to handle the case of an empty macro expansion given that a semi-colon is required. What's the difference between

if (true) {
  ;
}

and

if (true)
  ;

?

jdlangs gravatar image jdlangs  ( 2019-03-20 16:09:27 -0500 )edit

In this case if the macros were only one line then you could just rely on the semi-colon. As a defensive programming mechanism you should use the curly braces to make sure that your code executes correctly and is readable. https://index.ros.org/doc/ros2/Contri...

tfoote gravatar image tfoote  ( 2019-03-20 18:47:47 -0500 )edit

And you should always use the curly braces to make sure that you're robust to the macro being compiled out.

If compiled out they should be come an empty do {} while(0), not nothing.

William gravatar image William  ( 2019-03-20 19:04:10 -0500 )edit

Question Tools

2 followers

Stats

Asked: 2019-03-20 15:39:33 -0500

Seen: 929 times

Last updated: Mar 20 '19