[boost] [Review][bimap]

From: Thorsten Ottosen (thorsten.ottosen_at_[hidden])
Date: 2007-02-22 12:07:45

Hi Matias,

First of all I would like to say it's nice to see how well the SoC went.
Since  I was one who pushed for this library, I guess I should do a 
review. And it has been a pleasure to do so. I think this library
really shows the benefit of wrapping a fairly difficult library
(now we just need someone to do it for boost.graph :-)).

********************************************************
** I think this library should be accepted into boost! *
********************************************************

Here's a list of things I feel strongly about changing (see below for info):

- relation should have the same members as std::pair

- types should lie with members, so

   map::left_type::iterator i = m.left.begin()

instead of

   map::left_iterator i = m.left.begin()

- operator()[] seems way to complicatedly specified and does not follow 
the promise of mirroring STL. Keep the mutable operator()[] and then add

   value&       map.at( const key& );
   const value& map.at( const key& ) const;

which throws on lookup failure.

- reconsider modify/replace design: I suggest that you provide three 
functions:

   replace( it, key, value );
   replace_key( it, key );
   replace_value( it, value );

the last two should be optimally efficient and strongly exception-safe.

Great work Matias!                               *

-Thorsten

- What is your evaluation of the design?
----------------------------------------

Generally clean, sound and intuitive.

- What is your evaluation of the implementation?
------------------------------------------------

Did not look much.

- What is your evaluation of the documentation?
-----------------------------------------------

Really nice, maybe even a tad too graphical compared to the rest of boost.

Very thorough.

- What is your evaluation of the potential usefulness of the library?
---------------------------------------------------------------------

High.

- Did you try to use the library? With what compiler? Did you have any 
problems?
----------------------------------------------------------------------

No.

- How much effort did you put into your evaluation? A glance? A quick
reading? In-depth study?
---------------------------------------------------------------------

Several hours study of the documentation.

- Are you knowledgeable about the problem domain?
-------------------------------------------------

Fairly. I know a lot about datastructures and good design.

General comments
----------------

1. This statement:
"The relation class is a generalization of std::pair. The two values are 
named left and right to express the symmetry of this type."

It might be better to stay signature compatible with std::pair if you 
can. Boost.PtrMap initially started out without signature compatibility, 
but is now fixed to be that after user complaints.

Subsequently there is no need for a relation nested type as one can 
simply use value_type.

2. using namespace bimap; should be boost::bimap.

3. consider adding

      insert( const key&, const key2& )

   for performance and ease-of-use reasons.

4. In "Standard mapping framework"

"Note that the type of the collection of X is a set because we have 
chosen a map for the example"

This is somewhat comfusing to use "type". "The X (key) values form a set 
    of unique values" or something. The terminology could be sharper here.

5. "The two collections are then at the same level, so it is incorrect 
to name them as first`second` or `a`b. These names clearly impose an 
ordering on the two values." Again, I think it is more imporant for 
generic algortihms ect to stay with the std::pair layout.

6. Consider adding support for lookup/inserttion with types that are 
convertible to the key type (as a performance improvement).

7. "HashFunctor converts a T object into an integer value". Shouldn't 
that be "unsigned integer"?

8. "The code produced in this fashion tends ". It is not explained what 
"this fasion means". Put the definition earlier.

9. I find it somewhat assymetrical that in

   people.right.insert( People::right_value_type("Marta Smith",30215692) );

  the types come from People, but the function is called from .right.

Why are these types not avaiable as

People::LeftType::value_type

etc??

10. Could

   map_by<id>(people)[28928546]

be spelled

   people.by<id>()[28928546]

? It seems simpler to use and specify to me.

11. In

"    // Search the queried word on the from index (Spanish) */

     iterator_type_by<spanish,translator_bimap>::type is = 
map_by<spanish>(d).find(word);
"

where is d declared???

12. Should

     bool operator()(Rel ra, Rel rb)

not be

     bool operator()(Rel ra, Rel rb) const

??

13. In

   set_of_relation< RelOrder<_relation> >

_relation is not explained?

14. Consider addding

   value&       map.at( const key& );
   const value& map.at( const key& );

to complement operator[]() (The C++ language draft already has these)

15.  Should

   typedef bimap< int, std::string > bm_type;
   bm_type bm;
   bm[1] = "one";
   bm[2] = "two";

not be

   bm.left[1] = ...;

etc??

16. replace:

"The complexity is constant time if the changed element retains its 
original order with respect to all views; it is logarithmic otherwise."

Does the cimplexity not depend on the left and right type???

17. use namespace prefix foe boost::assign::list_of to avoid confusion.