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

From: Greg Colvin (gcolvin_at_[hidden])
Date: 2000-08-29 16:14:15

From: <jsiek_at_[hidden]>
> 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.

Can we not have both set() and operator[]?

> 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);
>           }
>           ...
>         };

This reads much better to me.
 
> (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]   
> ----------------------------------------------------------------------
> 
> 
> 
> 
>