[Documentation] [TitleIndex] [WordIndex

rospack/2008-09-30 Code Review

Package Developer:

Examiner:Rob Wheeler

Present at code review:

Notes from examiner

Documentation

Naming

Source Code

* Choose your containers wisely

The main data structure of rospack is a collection of packages. The two operations typically performed on the collection is to lookup a package by name and to insert a package into the collection.

When the filesystem is crawled to find packages it checks to see if the package name is already in use, and if not it inserts the package. This results in n lookups and n insertions.

Once the filesystem has been successfully crawled, the results can be stored in a cache. To load the cache, we simply do n insertions.

The most common operation is find. Here is how some different containers perform:

container

insertion

lookup

crawl

vector (unordered)

O(1)

O(n)

n*O(1) + n*O(n)

map

O(log n)

O(log n)

n*O(log n) + n*O(log n)

unordered_map (hash table)

O(1)

O(1)

n*O(1)+n*O(1)

Also, consider choices for:

  1. Priority queue
  2. Deque

* Containers of pointers to objects are clumsy and error-prone

* If it feels dirty when you write it, then it probably is

* When in Rome...

* Don't reinvent the wheel

* {X} Memory leak. The pointer newp is leaked:

* Streaming I/O can be cleaner. Instead of:

* Do the easy stuff first

* Don't optimized prematurely, but don't pessimize either...

* snarfing flags is confusing, error-prone, and difficult to verify. Boost::regex makes things easier. After defining the regular expression:

* Crawling for packages seems embarrassingly recursive

* Code to produce --profile is unnecessarily complicated:

* Are missing XML elements handled properly?

* Calling exit() makes valgrind unhappy if destructors need to be called.

Manifest / Build System

Testing

Conclusions

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



2019-12-14 13:04