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.