refactoring to make log4cxx an optional dependency #306
Conversation
|
Reviewing this is going to take a while, I'll try to get to it by Friday. |
|
Please take some time to review this in the near future so that we can pass it on for external testing/review. |
|
In the diff there are 20+ instances of "new" but no deletes. |
| color = COLOR_NORMAL; | ||
| } | ||
| else if (event->getLevel() == log4cxx::Level::getDebug()) | ||
| #if 0 |
There was a problem hiding this comment.
Should this block be left, or something else done with it.
There was a problem hiding this comment.
This code was just moved around. I would just keep it - I guess it was only used for debugging purposes.
|
@tfoote (#306 (comment)) The allocated objects are either globals which we currently have no way of cleaning up correctly as well as log appenders which are managed within log4cxx. |
|
I have integrated all changes. Any further feedback? Else I will hand it over for external review/testing. |
|
Nope looks good to me. |
|
+1 overall |
refactoring to make log4cxx an optional dependency
Fix viewer plugin relocation issue (ros#306)
refactoring to make log4cxx an optional dependency
@gerkey @tfoote @wjwwood Please review.
Especially any feedback on the interface for custom backends would be appreciated (see https://github.com/ros/ros_comm/compare/log4cxx?expand=1#diff-1e993bcf45a700893f4d675ec053f1aaR57).