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.

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

add get_backtrace method #79

Open
wants to merge 1 commit into
base: rolling
Choose a base branch
from
Open

Conversation

knorth55
Copy link

@knorth55 knorth55 commented Jun 7, 2023

Thank you for this great testing tools.
memory_tools is quite good to investigate dynamic memory allocation in RT thread.

In this PR, I add get_backtrace method to get backtrace in std::osstringstream format.
This method is useful when you want to use the stream with ROS logger like. (I'm using ROS1 now)

ROS_ERROR_STREAM("Backtrace:\n" << service.get_backtrace().str());

@knorth55 knorth55 requested a review from gbiggs as a code owner June 7, 2023 03:25
Copy link
Contributor

@clalancette clalancette left a comment

Choose a reason for hiding this comment

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

I think the idea behind this improvement is fine. I've left a couple of things to change.

@@ -93,6 +93,11 @@ struct MemoryToolsService
void
print_backtrace();

/// Returns a backtrace stream for logging.
OSRF_TESTING_TOOLS_CPP_MEMORY_TOOLS_PUBLIC
std::ostringstream
Copy link
Contributor

Choose a reason for hiding this comment

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

#include <sstream> at the top of the file for this.

/// Returns a backtrace stream for logging.
OSRF_TESTING_TOOLS_CPP_MEMORY_TOOLS_PUBLIC
std::ostringstream
get_backtrace();
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't love the name get_backtrace; it is unclear what you are getting. How about get_backtrace_stream? Also, I think this method can be const.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants