-
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
Changes from 3 commits
9a99a84
953db7a
6a0a6c7
2339df5
6683c47
1fb433f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -210,6 +210,27 @@ sub_will_underflow(const T x, const T y) | |
return (y > 0) && (x < (std::numeric_limits<T>::min() + y)); | ||
} | ||
|
||
/// Return the same string. | ||
/** | ||
* This function is overloaded to transform any string to C-style string | ||
* | ||
* \param[in] string_in is the string to be returned | ||
* \return The same string. | ||
*/ | ||
RCLCPP_PUBLIC | ||
const char * | ||
get_c_string(const char * string_in); | ||
|
||
/// Return the string, converted to C string | ||
/** | ||
* | ||
* \param[in] string_in is a std::string | ||
* \return The string tranformed to C string. | ||
*/ | ||
RCLCPP_PUBLIC | ||
const char * | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think you just need There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Added I also documented both functions and macros. |
||
get_c_string(const std::string & string_in); | ||
|
||
} // namespace rclcpp | ||
|
||
#endif // RCLCPP__UTILITIES_HPP_ |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -92,6 +92,23 @@ 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 commentThe 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 commentThe 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:
but never:
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 commentThe 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 |
||
} | ||
EXPECT_EQ(3u, g_log_calls); | ||
EXPECT_EQ("message three", g_last_log_event.message); | ||
|
||
RCLCPP_DEBUG(g_logger, "message " "four"); | ||
EXPECT_EQ("message four", g_last_log_event.message); | ||
|
||
RCLCPP_DEBUG(g_logger, std::string("message " "five")); | ||
EXPECT_EQ("message five", g_last_log_event.message); | ||
|
||
RCLCPP_DEBUG(g_logger, std::string("message %s"), "six"); | ||
EXPECT_EQ("message six", g_last_log_event.message); | ||
} | ||
|
||
TEST_F(TestLoggingMacros, test_logging_once) { | ||
for (int i : {1, 2, 3}) { | ||
RCLCPP_INFO_ONCE(g_logger, "message %d", 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.
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