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

From: Jens Maurer (Jens.Maurer_at_[hidden])
Date: 2000-09-03 17:43:32

Part five of my comments.   Finally some code inspection, but
not on all files.

Just in case I haven't said that before: I vote for adding the graph
library to boost.

General
-------

 - I think that every class or function in namespace boost should be
documented, except those in namespace detail.

 - We need to document BOOST_NO_STD_ITERATOR_TRAITS in a comment in
<boost/config.hpp>.

 - I second the idea to have separate reviews about some generally
useful stuff in GGCL.  For example, we get template metaprogramming
into boost, but we already had longish discussion about that.

Code
----
slist.hpp:  Does not have any copright/permission notice at all.
linked_list.hpp: Same here.
graph_structure.hpp:  incident() and opposite() are not documented in
the PDF, at least I could not find them with a text search.  And they
do not seem to be used anywhere.
tree_structure.hpp:  Same with traverse_tree().
boost/tie_example.cpp  should not be where it it is.
boost/tie_pair.hpp   should go away, use <boost/utility.hpp> instead.
  There are no backward compatibility issues, because it's the first
  integration into boost.
property_accessor.hpp:  If the enum ePropertyAccessorID is an
  implementation detail, move it to namespace detail.  Otherwise
  document it.
iterator_adaptors.hpp:   Is boost::type required to reside in namespace boost?
  boost::detail would be nicer.  boost::default_iterator_policies should be
  moved to namespace detail or be documented.  Same with boost::iterator_adaptor.
  #ifdef _MSC_VER is not correct to detect MSVC (only), use BOOST_MSVC instead.
Please check all your files for consistent copyright notices.
(LEDA.h, adjacency_iterator.h, adjacency_list.h don't have any,
array_binary_tree.h  has a long one, others have a short one.)
adjacency_list.h:  Make the one-argument constructors explicit.
  adjacency_list(const GraphPlugin& p) is not documented
  in 8.1.5.   Same with a few others.
array_binary_tree.h is in yet another namespace "adstl".  This should
be moved to boost and documented or moved to boost::detail.  It uses
GGCL_HAVE_ITERATOR_TRAITS, although it can never be defined when
array_binary_tree.h is the only header included by the user program.
base_visitor.h, meta_equal.h and meta_if.h have the "execute" bit
set after unpacking under Unix.  This does not seem useful.
bellman_ford.h:  The REQUIRE macros should use a prefix such as BOOST_*
utils.h:  The print_* functions should be documented or moved to
boost::detail.
tags.h: Can we #ifdef the enum noise for the particular compiler
deficiciency being worked around?
functor.h:  Lots of undocumented stuff.  And the names aren't very
descriptive in a general context.  The functionality is very
specific to property accessors and arrays.
Jens Maurer