rosconsole/2008-10-04 Code Review

Package Developer: Josh Faust

Examiner: Brian Gerkey

Present at code review:

Notes from examiner

Examiner: Enter your notes on each section below. See ExaminerChecklist for more detailed instructions.

• Divide your concerns up into bullet lists that can be run through in order in the meeting, and concretely decided about. Include enough details / relevant information for people at meeting to quickly decide on the issue (e.g. file and line number, description of race condition, or list unclear names)

• Use error symbol to call out things that are clear errors

• Use warning symbol to call out things that you're worried about

• Use check mark to mark areas that look good.

• Use the below bullets as a basic guide, but feel free to add new issues or remove issues that are not relevant (e.g. command-line options for a library)
• Feel free to delete this instruction section when you're done

Documentation

• API

  • The examples for ROS_DEBUG_COND_NAMED() and ROS_DEBUG_STREAM_COND_NAMED() don't pass in a name.

    • [josh] fixed

  • do_initialize() appears in the docs without any accompanying documentation. It looks like it's only for internal use. I'm surprised that Doxygen pulls it out from the source file. If it can't be hidden from Doxygen, it should be at least documented with "Don't call this directly."

    • [josh] fixed

  • I don't see anything about rosout. Is that an Appender yet to be written? More generally, how does one determine what goes to stdio, what goes to file, and what goes to rosout?

    • [josh] fixed -- Appender is written, but part of roscpp
• ROS API
  • n/a
• command-line
  • n/a
• tutorials / examples

  • Recommend a small self-contained example program that does some basic and some more advanced logging (e.g., changing logging levels on the fly).

    • [josh] fixed

  • The configuration example sets log4j.logger.ros. Should that be log4cxx instead of log4j? Also, this example implicitly suggests that log4j.logger.ros is the default ros logger. That (as well as other information regarding the logger hierarchy that we're using) should be documented explicitly somewhere.

    • [josh] fixed -- log4j is correct, but I added a note in the docs

Naming

• headers

  • 

• libraries

  • 

• exported symbols

  • namespace Levels is capitalized; should be levels.

    • [josh] fixed
• ROS API
  • n/a

Source Code

• header files

  • macros_generated.h should have a license statement (it will dirty the code generator, but make things clearer for the user).

    • [josh] fixed
• source files

  • Suggest moving python code generators into a subdirectory (perhaps src).

    • [josh] fixed
  • rosconsole.cpp:

    • What's the rationale for MAX_MESSAGE_SIZE being 4096? What happens if a longer message is logged?

      • [josh] fixed -- buffer starts at 4k, grows dynamically if necessary

    • Should stdio output (ROSConsoleStdioAppender::append) add a newline to the log message?

      • [josh] fixed -- also adds a prefix for the message type, and colors the text appropriately
• data / configuration / xml files
  • n/a

Manifest / Build System

• build

  • Builds, but with a bunch of warnings like this:

    [100%] Building CXX object CMakeFiles/rosconsole.dir/src/rosconsole/rosconsole.cpp.o
    /Users/gerkey/code/ros/3rdparty/log4cxx/log4cxx/include/log4cxx/helpers/objectptr.h: In copy constructor 'log4cxx::helpers::ObjectPtrT<T>::ObjectPtrT(const log4cxx::helpers::ObjectPtrT<T>&) [with T = log4cxx::Level]':
    /Users/gerkey/code/ros/3rdparty/log4cxx/log4cxx/include/log4cxx/level.h:55:   instantiated from here
    /Users/gerkey/code/ros/3rdparty/log4cxx/log4cxx/include/log4cxx/helpers/objectptr.h:74: warning: base class 'class log4cxx::helpers::ObjectPtrBase' should be explicitly initialized in the copy constructor

    These warnings should either be fixed in a patch to log4cxx, or the build should be altered to suppress them when compiling these headers. It will be a pain in the long run if everything compiled against rosconsole has a screenful of warnings.
  • [josh] fixed with a patch (and bug report submitted back to apache)
• dependencies
  • Made a small tweak to the build of log4cxx to make it work on OS X, but otherwise good.
  • Turns out that gtest's death tests only work on Linux, so I modified rosconsole's CMakeLists.txt to do the assertion test only on Linux.
• manifest data

  • 

Testing

• all tests pass

  • 

• unit tests

  • 

• rostests
  • n/a

Conclusions

This section will be filled out during the group code review meeting, and will include

• Passed! 1st package to clear!