{"action":"created","issue":{"url":"https://api.github.com/repos/ros2/rcutils/issues/113","repository_url":"https://api.github.com/repos/ros2/rcutils","labels_url":"https://api.github.com/repos/ros2/rcutils/issues/113/labels{/name}","comments_url":"https://api.github.com/repos/ros2/rcutils/issues/113/comments","events_url":"https://api.github.com/repos/ros2/rcutils/issues/113/events","html_url":"https://github.com/ros2/rcutils/issues/113","id":348337933,"node_id":"MDU6SXNzdWUzNDgzMzc5MzM=","number":113,"title":"Logging macros aren't safe in all contexts","user":{"login":"clalancette","id":163000,"node_id":"MDQ6VXNlcjE2MzAwMA==","avatar_url":"https://avatars0.githubusercontent.com/u/163000?v=4","gravatar_id":"","url":"https://api.github.com/users/clalancette","html_url":"https://github.com/clalancette","followers_url":"https://api.github.com/users/clalancette/followers","following_url":"https://api.github.com/users/clalancette/following{/other_user}","gists_url":"https://api.github.com/users/clalancette/gists{/gist_id}","starred_url":"https://api.github.com/users/clalancette/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/clalancette/subscriptions","organizations_url":"https://api.github.com/users/clalancette/orgs","repos_url":"https://api.github.com/users/clalancette/repos","events_url":"https://api.github.com/users/clalancette/events{/privacy}","received_events_url":"https://api.github.com/users/clalancette/received_events","type":"User","site_admin":false},"labels":[],"state":"open","locked":false,"assignee":null,"assignees":[],"milestone":null,"comments":10,"created_at":"2018-08-07T14:08:26Z","updated_at":"2018-08-07T19:54:09Z","closed_at":null,"author_association":"CONTRIBUTOR","body":"Consider the following structure:\r\n\r\n```\r\nif (condition)\r\n    RCUTILS_LOG_ERROR(\"error\");\r\nelse\r\n{\r\n    RCUTILS_LOG_INFO(\"info\");\r\n}\r\n```\r\n\r\nWith the current ROS2 rcutils, this code will fail to compile with something like:\r\n\r\n```\r\n/home/ubuntu/teleop_twist_ws/src/badmacro/src/badmacro.cpp: In member function ‘int BadMacro::main()’:\r\n/home/ubuntu/teleop_twist_ws/src/badmacro/src/badmacro.cpp:21:7: error: expected ‘}’ before ‘else’\r\n       else\r\n       ^~~~\r\nIn file included from /opt/ros/bouncy/include/rclcpp/client.hpp:39:0,\r\n                 from /opt/ros/bouncy/include/rclcpp/callback_group.hpp:23,\r\n                 from /opt/ros/bouncy/include/rclcpp/node_interfaces/node_base_interface.hpp:25,\r\n                 from /opt/ros/bouncy/include/rclcpp/executor.hpp:31,\r\n                 from /opt/ros/bouncy/include/rclcpp/executors/multi_threaded_executor.hpp:24,\r\n                 from /opt/ros/bouncy/include/rclcpp/executors.hpp:21,\r\n                 from /opt/ros/bouncy/include/rclcpp/rclcpp.hpp:144,\r\n                 from /home/ubuntu/teleop_twist_ws/src/badmacro/src/badmacro.cpp:3:\r\n```\r\n\r\nI have a minimal reproducer of this problem over at https://github.com/clalancette/badmacro .  We probably need to wrap all of these macros in the `do { ... } while(0)` construct to make them safe in these circumstances."},"comment":{"url":"https://api.github.com/repos/ros2/rcutils/issues/comments/411180913","html_url":"https://github.com/ros2/rcutils/issues/113#issuecomment-411180913","issue_url":"https://api.github.com/repos/ros2/rcutils/issues/113","id":411180913,"node_id":"MDEyOklzc3VlQ29tbWVudDQxMTE4MDkxMw==","user":{"login":"clalancette","id":163000,"node_id":"MDQ6VXNlcjE2MzAwMA==","avatar_url":"https://avatars0.githubusercontent.com/u/163000?v=4","gravatar_id":"","url":"https://api.github.com/users/clalancette","html_url":"https://github.com/clalancette","followers_url":"https://api.github.com/users/clalancette/followers","following_url":"https://api.github.com/users/clalancette/following{/other_user}","gists_url":"https://api.github.com/users/clalancette/gists{/gist_id}","starred_url":"https://api.github.com/users/clalancette/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/clalancette/subscriptions","organizations_url":"https://api.github.com/users/clalancette/orgs","repos_url":"https://api.github.com/users/clalancette/repos","events_url":"https://api.github.com/users/clalancette/events{/privacy}","received_events_url":"https://api.github.com/users/clalancette/received_events","type":"User","site_admin":false},"created_at":"2018-08-07T19:54:09Z","updated_at":"2018-08-07T19:54:09Z","author_association":"CONTRIBUTOR","body":"> But it should prevent moving forward on this item since the decision is pretty much orthogonal (if a change is made the whole code base would need to be updated accordingly).\r\n\r\nUnfortunately, it is not completely orthogonal.  The problem is that we have two choices on how to update the macro:\r\n\r\n1.  `do { ... } while(0);` - This will work with the current codebase, but will cause anywhere that *does* use `RCUTILS_LOG_ERROR();` to have a double semicolon.  There are examples of this in the ROS2 core (https://github.com/ros2/demos/blob/master/demo_nodes_cpp_native/src/talker.cpp#L47), plus we will almost certainly have examples that come in during ports.  These are (potentially) vulnerable to the problem that I pointed out in point 2 of https://github.com/ros2/rcutils/issues/113#issuecomment-411162427.\r\n1.  `do { ... } while(0)` - This will not compile with the current codebase, since we'll have unterminated statements all over the place.  In my opinion, this is actually the correct thing to do; it makes it so that the macros are valid in basically all circumstances.  While our linters and style can call for not using the semicolon, those will not apply to downstream packages which are now potentially unsafe."}}
{"actor":{"display_login":"clalancette"}}