Providing logging macro signature that accepts std::string by fmrico · Pull Request #573 · ros2/rclcpp

fmrico · 2018-10-21T18:55:36Z

Hi all,

This commit provides a way to use logging macros with std::string.

I hope it is ok for you :)

Best
Francisco

Connects to #570

wjwwood

I had two small changes to request, but otherwise lgtm!

Sorry, I started the review but GitHub's issues yesterday prevented me from following through until now.

wjwwood · 2018-10-22T00:26:01Z

rclcpp/resource/logging.hpp.em

 #define RCLCPP_LOG_MIN_SEVERITY_NONE 5

+#define FIRST_ARG(N, ...) N
+#define ALL_BUT_FIRST_ARGS(N, ...) , ##__VA_ARGS__


Can you please prefix these with RCLCPP_ so they don't accidentally collide with other software?

wjwwood · 2018-10-22T00:27:02Z

rclcpp/test/test_logging.cpp


+TEST_F(TestLoggingMacros, test_logging_string) {
+  for (std::string i : {"one", "two", "three"}) {
+    RCLCPP_DEBUG(g_logger, "message " + i);


Can you add at least one test case that uses multiple arguments, so that the "all but first argument" style macros are being exercised?

I have added new tests, with more than one argument, but I understand that possible uses are:

RCLCPP_DEBUG(g_logger, std::string("message " + i));

RCLCPP_DEBUG(g_logger, std::string("message " "one"));

RCLCPP_DEBUG(g_logger, std::string("message %d"), 1); //This works, but I am not sure if it is correct

RCLCPP_DEBUG(g_logger, std::string("message %d %d %d %d"), 1, 2, 3, 4)); //This works, but I am not sure if it is correct

RCLCPP_DEBUG(g_logger, "message %d %d %d %d", 1, 2, 3, 4)); // standard use

but never:

RCLCPP_DEBUG(g_logger, std::string("message "), std::string("one"));

Only first argument is possible to be a string. If I am wrong, let me know, because I am not really sure if this can happen...

That looks right to me, the documentation as it stands is that the first string is a format string (which may have no format arguments) and the rest of the arguments are input for the format string:

http://docs.ros2.org/bouncy/api/rclcpp/logging_8hpp.html#a44818c8a1fd292cd0e81759e956765d7

- New tests added

wjwwood

LGTM! Thanks for iterating. I'll run CI before merging.

wjwwood · 2018-10-23T22:56:43Z

CI:

Linux
Linux-aarch64
macOS
Windows

wjwwood · 2018-10-23T23:01:51Z

I'll merge if CI passes, but in the mean time, if you're willing it would be good to document this somewhere.

You might consider modifying the API docs (which are extracted, e.g.: http://docs.ros2.org/bouncy/api/rclcpp/logging_8hpp.html#a44818c8a1fd292cd0e81759e956765d7) which are in the template here:

https://github.com/ros2/rclcpp/pull/573/files#diff-8b70a08e4756e73f75947864f9f9c8e4R87

Or you might add a note to the wiki, we don't have comprehensive user documentation on logging just yet, but this wiki page is the closest thing:

https://github.com/ros2/ros2/wiki/Logging#logging-usage

Thanks again.

wjwwood · 2018-10-23T23:48:05Z

Looks like an issue on Windows:

23:31:28   test_service.obj : error LNK2019: unresolved external symbol "char const * __cdecl rclcpp::get_c_string(char const *)" (?get_c_string@rclcpp@@YAPEBDPEBD@Z) referenced in function "public: __cdecl <lambda_28aaba8dfd9ad090ce1fa0449fa20263>::operator()(struct rcl_service_t *)const " (??R<lambda_28aaba8dfd9ad090ce1fa0449fa20263>@@QEBA@PEAUrcl_service_t@@@Z) [C:\J\workspace\ci_windows\ws\build\rclcpp\test_service.vcxproj]
23:31:28   C:\J\workspace\ci_windows\ws\build\rclcpp\Release\test_service.exe : fatal error LNK1120: 1 unresolved externals [C:\J\workspace\ci_windows\ws\build\rclcpp\test_service.vcxproj]

I think it either needs to set the visibility (dllimport/dllexport) or maybe make it an inline header function. I'll make an inline comment.

wjwwood · 2018-10-23T23:48:38Z

rclcpp/include/rclcpp/utilities.hpp

+const char *
+get_c_string(const char * string_in);
+
+const char *


I think you just need RCLCPP_PUBLIC on each of these functions.

Added RCLCPP_PUBLIC to get_c_string functions. I didn't know that it was necessary for Windows. I use only Linux :S . I hope it works with these changes.

I also documented both functions and macros.

- Functions declared as RCLCPP_PUBLIC

fmrico · 2018-10-24T04:50:16Z

Thank you @wjwwood for your comment in this PR !!

dirk-thomas · 2018-10-24T05:29:51Z

rclcpp/include/rclcpp/utilities.hpp

+/**
+ *
+ * \param[in] string_in is a std::string
+ * \return The string tranformed to C string.


Spelling: tranformed

uhhh, it is true. Sorry :S

Fixed

fmrico · 2018-10-24T16:46:12Z

Hi @wjwwood ,

Is it possible to run CI to see if compilation in Windows and macOS works?

Thanks!!

clalancette · 2018-10-24T17:07:39Z

@fmrico I kicked off another round of CI for you:

Linux
Linux-aarch64
macOS
Windows

wjwwood · 2018-10-25T04:55:36Z

Thanks @clalancette.

@fmrico looks like another issue is that the syntax you're using in the macro is a GNU extension (which clang complains about on macOS):

18:45:28 In file included from /Users/osrf/jenkins-agent/workspace/ci_osx/ws/src/ros2/rviz/rviz2/test/tools/send_lots_of_points_node.cpp:33:
18:45:28 In file included from /Users/osrf/jenkins-agent/workspace/ci_osx/ws/install/rclcpp/include/rclcpp/rclcpp.hpp:144:
18:45:28 In file included from /Users/osrf/jenkins-agent/workspace/ci_osx/ws/install/rclcpp/include/rclcpp/executors.hpp:21:
18:45:28 In file included from /Users/osrf/jenkins-agent/workspace/ci_osx/ws/install/rclcpp/include/rclcpp/executors/multi_threaded_executor.hpp:24:
18:45:28 In file included from /Users/osrf/jenkins-agent/workspace/ci_osx/ws/install/rclcpp/include/rclcpp/executor.hpp:31:
18:45:28 In file included from /Users/osrf/jenkins-agent/workspace/ci_osx/ws/install/rclcpp/include/rclcpp/node_interfaces/node_base_interface.hpp:25:
18:45:28 In file included from /Users/osrf/jenkins-agent/workspace/ci_osx/ws/install/rclcpp/include/rclcpp/callback_group.hpp:24:
18:45:28 In file included from /Users/osrf/jenkins-agent/workspace/ci_osx/ws/install/rclcpp/include/rclcpp/service.hpp:33:
18:45:28 /Users/osrf/jenkins-agent/workspace/ci_osx/ws/install/rclcpp/include/rclcpp/logging.hpp:35:45: error: token pasting of ',' and __VA_ARGS__ is a GNU extension [-Werror,-Wgnu-zero-variadic-macro-arguments]
18:45:28 #define RCLCPP_ALL_BUT_FIRST_ARGS(N, ...) , ##__VA_ARGS__
18:45:28                                             ^
18:45:28 /Users/osrf/jenkins-agent/workspace/ci_osx/ws/install/rclcpp/include/rclcpp/logging.hpp:35:45: error: token pasting of ',' and __VA_ARGS__ is a GNU extension [-Werror,-Wgnu-zero-variadic-macro-arguments]
18:45:28 2 errors generated.
18:45:28 make[2]: *** [test/tools/CMakeFiles/send_lots_of_points_node.dir/send_lots_of_points_node.cpp.o] Error 1
18:45:28 make[1]: *** [test/tools/CMakeFiles/send_lots_of_points_node.dir/all] Error 2
18:45:28 make[1]: *** Waiting for unfinished jobs....
18:45:28 make: *** [all] Error 2

Off the top of my head I don't know what the right thing to do here is. Maybe you could investigate Google results or stackoverflow for some insight?

It looks like a warning (we're using -Werror in a few places, so that's why it fails the build).

Sorry for the back and forth, it's the double edged sword of portability...

fmrico · 2018-10-25T16:24:23Z

Thanks @wjwwood I will try to find a solution. I have macOS in my computer, so I will work hard on this

fmrico · 2018-10-25T19:35:43Z

@wjwwood The solution was almost trivial :)

Fixed in 6683c47

Built and tested both in Ubuntu (Bionic, gcc) and macOSX (Capitan, clang)

wjwwood · 2018-10-25T20:17:05Z

Thanks I'll run new CI:

Linux
Linux-aarch64
macOS
Windows

wjwwood

lgtm! Thanks for iterating.

The CI is still yellow, but those are known upstream warnings we're walking on in parallel. So we're good to go.

* Providing logging macro signature that accepts std::string * - RCLCPP_ prefix to macros Add - New tests added * - Added doc to the functions and macros - Functions declared as RCLCPP_PUBLIC * - Small typo in doc corrected * Fixed error when compiling with clang * touch up docs

Providing logging macro signature that accepts std::string

9a99a84

tfoote added the in review Waiting for review (Kanban column) label Oct 21, 2018

wjwwood self-assigned this Oct 22, 2018

wjwwood mentioned this pull request Oct 22, 2018

provide logging macro signature that accepts std::string #570

Closed

wjwwood requested changes Oct 23, 2018

View reviewed changes

- RCLCPP_ prefix to macros Add

953db7a

- New tests added

wjwwood approved these changes Oct 23, 2018

View reviewed changes

wjwwood reviewed Oct 23, 2018

View reviewed changes

- Added doc to the functions and macros

6a0a6c7

- Functions declared as RCLCPP_PUBLIC

fmrico closed this Oct 24, 2018

tfoote removed the in review Waiting for review (Kanban column) label Oct 24, 2018

fmrico reopened this Oct 24, 2018

tfoote added the in review Waiting for review (Kanban column) label Oct 24, 2018

dirk-thomas reviewed Oct 24, 2018

View reviewed changes

- Small typo in doc corrected

2339df5

Fixed error when compiling with clang

6683c47

touch up docs

1fb433f

wjwwood approved these changes Oct 25, 2018

View reviewed changes

wjwwood merged commit b600c18 into ros2:master Oct 25, 2018

wjwwood removed the in review Waiting for review (Kanban column) label Oct 25, 2018

fmrico deleted the logging_string branch October 25, 2018 23:16

Conversation

fmrico commented Oct 21, 2018 • edited by wjwwood Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

wjwwood left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wjwwood left a comment

Choose a reason for hiding this comment

Uh oh!

wjwwood commented Oct 23, 2018

Uh oh!

wjwwood commented Oct 23, 2018

Uh oh!

wjwwood commented Oct 23, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fmrico Oct 24, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fmrico commented Oct 24, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fmrico commented Oct 24, 2018

Uh oh!

clalancette commented Oct 24, 2018

Uh oh!

wjwwood commented Oct 25, 2018

Uh oh!

fmrico commented Oct 25, 2018

Uh oh!

fmrico commented Oct 25, 2018

Uh oh!

wjwwood commented Oct 25, 2018

Uh oh!

wjwwood left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

fmrico commented Oct 21, 2018 •

edited by wjwwood

Loading

fmrico Oct 24, 2018 •

edited

Loading