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

From: jsiek_at_[hidden]
Date: 2000-08-29 16:08:06

David Abrahams writes:
 >  1. Rename "property accessor" to "property tag", "property reference", or
 >     probably best of all, simply "property". I know there were long
 >     discussions about this in the past, but consider: the way the usage
 >     above looks, the property accessor isn't really doing any accessing at
 >     all. Instead, it is the put() function which accesses the property.

put() really does "belong" to the property accessor object. It would
normally be a member function but is more flexible as a free function.
For example, it can be overloaded for pointers. Then when vertices are
just integers you can use them to offset into the array.

 >  2. Rename "put()" to "set()". The code above seems to be setting the
 >     predecessor property of the target node.

The name was originally set() which caused conflicts with std::set on
VC++ when the user does a "using namespace set" and then includes ggcl
headers, as in the following case:

#include <set>
using namespace set;
namespace boost {
  void set() { }
}

Of course, my preference is still not to use put() or set(), but
to use the operator[]. This looses the flexibility of a free function,
but it is very easy to read and automatically works for pointer types.
Renaming _p to _predecessor, and changing to operator[] gives
the following code:

        template <class Predecessor>
        class predecessor_visitor
        {
          ...
          typedef tree_edge_tag category;

          template <class Edge, class Graph>
          void process(Edge e, Graph& g) {
            _predecessor[target(e, g)] = source(e, g);
          }
          ...
        };

(the GGCL algorithms internally still use operator[], which turns
into a call to put/get through a wrapper)

 >  3. Rename "category" to "filter" (or something). At least if I've
 >     understood your categories properly, they don't mirror categories as
 > we've seen
 >     them for iterators and containers (describing capabilities).

how about edge_filter

 >  4. Rename "tree_edge_tag" to something which better describes its function,
 >     like "minimum_cost_path_filter".

It would only be the minimum cost when used with a best-first kind of
search, but you might be doing a plain depth-first or breadth-first
search. Perhaps the names "search_tree_edge_tag" or
"edge_in_search_tree_tag" would be better?

 > You continue:
 > So, does this function "visit a predecessor", or make an object which does
 > so? If I've understood things, the answer is no. Instead, I think it makes
 > an object which sets the "predecessor in a shortest path" property. So it
 > should be called something like "label_shortest_path".

how about predecessor_recorder for the class and record_predecessors()
for the function name?

Thanks for the suggestions!

Jeremy

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