Re: [boost] [Review] Boost Graphs Library (GGCL)

From: Jens Maurer (Jens.Maurer_at_[hidden])
Date: 2000-08-28 17:05:52

Here are my review comments.

I'm going to bed for now.  I'll add some more tomorrow.

General
-------

 - LICENSE: I know the license was changed already to fit in the boost
scheme.  Notwithstanding, could Beman please check whether the restriction
on modification (paragraph 3) are compatible with the boost license
policy?  I don't think there's a problem, though.

 - The header directory <boost/> contains a lot of headers which are
already distributed with boost.  This makes it more difficult to find
the new stuff.

 - I am not sure I understand the relationship between the new headers
added to <boost/> and those in <ggcl/>.  I thought that all headers
should go under <boost/>, probably in a subdirectory such as
<boost/graph/> if it gets too much.

 - boost has decided on .hpp as its header extension. <ggcl/> contains
.h extensions instead.

Concepts
--------

 - Having an "edge descriptor" is a good thing, iterators alone
are not sufficient in my experience.

Documentation (ggcl_user_manual-2.2.pdf)
----------------------------------------

 - 1.2.1: I discourage "using namespace XXX" in documentation and code.
 - 1.2.1: <ggcl/bidir_adj_list.h> does not exist.
 - 1.2.1: If you don't need argc and argv in main(), just write "int main()"
and be done with it.
 - 1.2.3: Most functions such as tie() and index() are not available with
the previously shown #include files.
 - 1.2.4: Vertex Descriptors: Is there any reason not to pass "Vertex"
as "const &" to operator() ?  If so, please state that explicitly.
 - Table 5.1: I cannot see why a VertexListGraph refines AdjacencyGraph.
In my opinion, iterating though some graph's vertex set is completely
orthogonal to any adjacency properties.
   Similarly, there may be algorithms which only need the in-edges, but
not the out-edges. Tying the two properties together in a
BidirectionalGraph is probably not helpful.
 - 5.1.1: Graph seems to be required to support a default constructor.
At least IncidenceGraph's concept checking class says that and it
doesn't add a "DefaultConstructible" constraint.  Please document it.
 - 5.1.13: How is a MultiPassInputIterator different from a
ForwardIterator?

Headers
-------

boost/graph_structure.hpp:
 - There is some commented-out cruft there which I cannot make any
sense of in that context.
 - The incident() method uses source() and target(), which are not
declared at this point.
 - Shouldn't opposite() throw an exception when used incorrectly
instead of silently returning an empty Edge, which hasn't been declared,
anyway?
 - The longish #if 0 stuff should be removed.

Jens Maurer