Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Providing logging macro signature that accepts std::string #573

Merged
merged 6 commits into from
Oct 25, 2018
Merged

Providing logging macro signature that accepts std::string #573

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
9a99a84
Providing logging macro signature that accepts std::string
fmrico Oct 21, 2018
953db7a
- RCLCPP_ prefix to macros Add
fmrico Oct 23, 2018
6a0a6c7
- Added doc to the functions and macros
fmrico Oct 24, 2018
2339df5
- Small typo in doc corrected
fmrico Oct 24, 2018
6683c47
Fixed error when compiling with clang
fmrico Oct 25, 2018
1fb433f
touch up docs
wjwwood Oct 25, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions rclcpp/include/rclcpp/utilities.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -210,6 +210,12 @@ sub_will_underflow(const T x, const T y)
return (y > 0) && (x < (std::numeric_limits<T>::min() + y));
}

const char *
get_c_string(const char * string_in);

const char *
Copy link
Member

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.

Copy link
Contributor Author

@fmrico fmrico Oct 24, 2018 •
edited
Loading

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.

get_c_string(const std::string & string_in);

} // namespace rclcpp

#endif // RCLCPP__UTILITIES_HPP_
6 changes: 5 additions & 1 deletion rclcpp/resource/logging.hpp.em
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@

#include "rclcpp/logger.hpp"
#include "rcutils/logging_macros.h"
#include "rclcpp/utilities.hpp"

// These are used for compiling out logging macros lower than a minimum severity.
#define RCLCPP_LOG_MIN_SEVERITY_DEBUG 0
Expand All @@ -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__
Copy link
Member

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done


/**
* \def RCLCPP_LOG_MIN_SEVERITY
* Define RCLCPP_LOG_MIN_SEVERITY=RCLCPP_LOG_MIN_SEVERITY_[DEBUG|INFO|WARN|ERROR|FATAL]
Expand Down Expand Up @@ -94,7 +98,7 @@ def is_supported_feature_combination(feature_combination):
@(''.join([' ' + p + ', \\\n' for p in params]))@
@[ end if]@
logger.get_name(), \
__VA_ARGS__)
rclcpp::get_c_string(FIRST_ARG(__VA_ARGS__, "")) ALL_BUT_FIRST_ARGS(__VA_ARGS__,""))

@[ end for]@
#endif
Expand Down
12 changes: 12 additions & 0 deletions rclcpp/src/rclcpp/utilities.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -353,3 +353,15 @@ rclcpp::sleep_for(const std::chrono::nanoseconds & nanoseconds)
// Return true if the timeout elapsed successfully, otherwise false.
return !g_is_interrupted;
}

const char *
rclcpp::get_c_string(const char * string_in)
{
return string_in;
}

const char *
rclcpp::get_c_string(const std::string & string_in)
{
return string_in.c_str();
}
8 changes: 8 additions & 0 deletions rclcpp/test/test_logging.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Copy link
Member

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?

Copy link
Contributor Author

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...

Copy link
Member

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

}
EXPECT_EQ(3u, g_log_calls);
EXPECT_EQ("message three", 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);
Expand Down