-
Notifications
You must be signed in to change notification settings - Fork 412
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.
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
Providing logging macro signature that accepts std::string #573
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
rclcpp/resource/logging.hpp.em
Outdated
@@ -30,6 +31,9 @@ | |||
#define RCLCPP_LOG_MIN_SEVERITY_FATAL 4 | |||
#define RCLCPP_LOG_MIN_SEVERITY_NONE 5 | |||
|
|||
#define FIRST_ARG(N, ...) N | |||
#define ALL_BUT_FIRST_ARGS(N, ...) , ##__VA_ARGS__ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you please prefix these with RCLCPP_
so they don't accidentally collide with other software?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
@@ -92,6 +92,14 @@ TEST_F(TestLoggingMacros, test_logging_named) { | |||
EXPECT_EQ("message 3", g_last_log_event.message); | |||
} | |||
|
|||
TEST_F(TestLoggingMacros, test_logging_string) { | |||
for (std::string i : {"one", "two", "three"}) { | |||
RCLCPP_DEBUG(g_logger, "message " + i); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add at least one test case that uses multiple arguments, so that the "all but first argument" style macros are being exercised?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Thanks for iterating. I'll run CI before merging.
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. |
Looks like an issue on Windows:
I think it either needs to set the visibility ( |
const char * | ||
get_c_string(const char * string_in); | ||
|
||
const char * |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you just need RCLCPP_PUBLIC
on each of these functions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
Thank you @wjwwood for your comment in this PR !! |
rclcpp/include/rclcpp/utilities.hpp
Outdated
/** | ||
* | ||
* \param[in] string_in is a std::string | ||
* \return The string tranformed to C string. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Spelling: tranformed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
uhhh, it is true. Sorry :S
Fixed
Hi @wjwwood , Is it possible to run CI to see if compilation in Windows and macOS works? Thanks!! |
@fmrico I kicked off another round of CI for you: |
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):
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 Sorry for the back and forth, it's the double edged sword of portability... |
Thanks @wjwwood I will try to find a solution. I have macOS in my computer, so I will work hard on this |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
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