Skip to content
This repository was archived by the owner on May 31, 2025. It is now read-only.

Add deregistry function for LogAppender.#17

Merged
dirk-thomas merged 7 commits intoros:melodic-develfrom
tahsinkose:melodic-devel
Sep 11, 2018
Merged

Add deregistry function for LogAppender.#17
dirk-thomas merged 7 commits intoros:melodic-develfrom
tahsinkose:melodic-devel

Conversation

@tahsinkose
Copy link
Copy Markdown
Contributor

Resolution of memory leak issue in ROSOutAppender requires deregistration of g_rosout_appender. This PR implements that functionality.

@dirk-thomas dirk-thomas added the enhancement New feature or request label Sep 11, 2018
@dirk-thomas
Copy link
Copy Markdown
Member

The same function needs to be added to the other backends.

Add deregistry to other backends as well.
@tahsinkose
Copy link
Copy Markdown
Contributor Author

I don't know why build fails, Jenkins shows nothing about the failure. I thought it was because of multiple commits, so squashed them but didn't work either.

@dirk-thomas
Copy link
Copy Markdown
Member

The Jenkins build output contains:

[ 50%] Building CXX object CMakeFiles/rosconsole_log4cxx.dir/src/rosconsole/impl/rosconsole_log4cxx.cpp.o
/tmp/catkin_workspace/src/rosconsole/src/rosconsole/impl/rosconsole_log4cxx.cpp: In function ‘void ros::console::impl::deregister_appender()’:
/tmp/catkin_workspace/src/rosconsole/src/rosconsole/impl/rosconsole_log4cxx.cpp:358:3: error: ‘logger’ was not declared in this scope
   logger->removeAppender(g_log4cxx_appender);
   ^~~~~~
/tmp/catkin_workspace/src/rosconsole/src/rosconsole/impl/rosconsole_log4cxx.cpp:358:3: note: suggested alternative: ‘log1pq’
   logger->removeAppender(g_log4cxx_appender);
   ^~~~~~
   log1pq

@tahsinkose
Copy link
Copy Markdown
Contributor Author

Thanks for your patient guidance :)

rosconsole_glog_appender = appender;
}

void deregister_appender(){
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Please follow the same style of the file: putting the { in a new line.

Same below.

const log4cxx::LoggerPtr& logger = log4cxx::Logger::getLogger(ROSCONSOLE_ROOT_LOGGER_NAME);
logger->removeAppender(g_log4cxx_appender);
delete g_log4cxx_appender;
g_log4cxx_appender = 0;
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Please follow the same style of the file: 2 space indentation (no tabs).

Same below.

}

void deregister_appender(){
rosconsole_glog_appender = NULL;
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Please use 0 here for consistency - that is the default value further up in the same file and in the other implementations.

@dirk-thomas
Copy link
Copy Markdown
Member

Thank you for adding this feature.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants