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

From: jsiek_at_[hidden]
Date: 2000-08-28 19:43:29

Jens Maurer writes:
 > 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.

Very little in the boost/ directory is new stuff, mostly just the
traits classes in graph_structure.hpp and property_accessors.hpp.
Also added tie to utility.hpp.

 >  - 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.

I was planning on making these changes once GGCL is accepted 
(as well as moving everything from the ggcl namespace to the boost
 namespace).

 > 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.

The bidir_adj_list class has been deprecated and the functionality
folded into the adjacency_list class. I'll make sure to update the
docs.

 >  - 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.

No reason, that can change.

 >  - 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.

The reason I had for that is that a vertex set without any adjacency
properties is no longer really a graph, it's nothing more than a set.
Also, I don't know of any graph algorithm that iterates though the
vertices without also looking at out-edges.

 >    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.

That may be true. I'll look into that.

 >  - 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.

What made you think the default constructor was required? The fact
that the concept checking class had the graph as a data member?  This
doesn't induce the requirement for a default constructor because
instances of concept checking classes are never created. I'll make
sure this is explained in the docs.

 >  - 5.1.13: How is a MultiPassInputIterator different from a
 > ForwardIterator?

There is very little difference. The only difference is that a
ForwardIterator requires the return type of operator*() to be T&,
whereas MultiPassInputIterator is like InputIterator in that the
reference type merely has to be convertible to T. I'll make
sure this is added to the docs.

 > 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.

incident() is a template function, so this does not matter.

 >  - Shouldn't opposite() throw an exception when used incorrectly
 > instead of silently returning an empty Edge, which hasn't been declared,
 > anyway?

Yes, an exception would be good here.

 >  - The longish #if 0 stuff should be removed.

Will do.

Thanks for the comments!

Jeremy

----------------------------------------------------------------------
 Jeremy Siek                        www: http://www.lsc.nd.edu/~jsiek/
 Ph.D. Candidate                  email: jsiek_at_[hidden]
 Univ. of Notre Dame         cell phone: [phone]   
----------------------------------------------------------------------