Boost logo

Boost :

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& const key& );
   const value& const key& ) const;

which throws on lookup failure.

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

   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! *


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


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


- 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



10. Could


be spelled<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 =

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& const key& );
   const value& 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] = ...;


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.

Boost list run by bdawes at, gregod at, cpdaniel at, john at