Hi Giovanni! On 3/3/07, Giovanni Piero Deretta <gpderetta@gmail.com> wrote:
Looks like I didn't make it in time.
Do not worry, It will be useful anyway :)
Ion just announced the result, and I congratulate with Matias for the result of the review. I'll add here my (short) review of the library anyways.
Thanks!
First of all, I think that the library should be accepted, but that doesn't matter anyway now :)
Yes, it matters.
* What is your evaluation of the design?
The design looks sound. Probably the interface is a bit complex for being a simplified wrapper around MI, but on the other hand, Bimap is certainly not a simple wrapper. Still the most common use is bimap<A, B>, and Boost.Bimap makes the simple case simple.
Ok, that is an important thing. If bimap<a,b> is complicated we have screw the idea of the library big time.
I do not buy much the reason for preferring left/right to first/second. At least in an std::pair, their name does not reflect the importance in the sequence (as it might be in an std::map), but just their position in the signature. For the same reason, bmap could use these tags to name the left and right view.
The discussion was about the names for the relation class. I do not think that first/second notation are good names for the views because the resulting code will look like: bm.first.begin()->second bm_type::second_iterator ... The idea of left/right notation for the views stand for the "left view", or the map seeing by the left side. I think that the "first view" or the "first map" is confusing.
The tutorial states:
"Be aware that a bidirectional map is only signature-compatible with standard containers. Some functions may give different results, such as in the case of inserting a pair into the left map where the second value conflicts with a stored relation in the container. The functions may be slower in a bimap because of the duplicated constraints. It is strongly recommended that you read The full tutorial if you intend to use a bimap in a serious project."
Wouldn't be better to make functions with different behaviour from standard containers *not* signature compatible? I've been bitten at times with function signature compatibility but behaviour incompatibility in standard containers.
The thing is that the difference in general can
The lambda support names the placeholder for the left, right, first, second and key with a underscore in the beginning (i.e. "_first"). While I think this is done to mimic the "_1" notation, I think that here the underscore is not really necessary. Probably just "first" would just be better. Also shouldn't support for pair be directly inside boost::lambda? (speaking of it, can we also have built in support for tuples in lambda? Pleeease).
IMO first is a very common name and will crush a lot in user code. This is something that we have not discussed during the review. The other option is to use first_ and second_.
* What is your evaluation of the implementation?
Didn't study it enough to make a comment.
* What is your evaluation of the documentation?
Very Good.
It is very clean and I like the graphic aspect, while maintaining a Boost look and feel. The pictures used to explain relations help a lot. I also like the color scheme.
Too much green :)
The index page has a very nice image based menu. While it looks great, it makes it harder to navigate it using the "Find as you type" functionality of Firefox. I use this functionality a lot when navigating documentation. A test based duplicate could be exposed in the page.
Maybe at the end of the page this menu can be added.
The one minute tutorial explains very well the the basic bimap<A, B> interface.
I have been told to extend it because it leaves a few holes. I will try not to clutter it unnecessarily.
I've found parts of the extended tutorial a bit non intuitive. In particular the part about "The set of relations type". I had to read it a couple of times, and I do not feel that I still get it completely. But that Is probably just me.
No it is not just you. Other reviewers have problems there too. I have to change that page, and if then there are still problems reconsider a design change in that area.
The documentation assumes that this is in effect.
using namespace bimap;
As many have stated before in reviews of other libraries, I think that something like:
namespace bm = boost::bimap;
would be more appropriate. I think this is not just a matter of style. When reading documentations, I often find myself looking up symbols to see in what namespace are supposed to be (is this symbol from bimap, was it part of another library, is it a local typedef?).
I do not know... the code will end looking like: bm::bimap< bm::set_of< bm::tagged<A, TA>, bm::multi_set_of< double > > And all the bm::s will distract the reader.
Tagging:
"member_at::{side}"
Here "{side}" is a placeholder for "left/right". But "{" and "}" have meaning in c++, and it took me a while not to parse it as valid code :( . This placeholder syntax is used in other places in the documentation. Probably some other typographic convention should be used.
What about member_at::-side- or member_at::#side# ?
The reference section is impressive, with detailed explanation for every member function. Great job.
All the glory should go to Joaquin. That section is based in the impressive docs of Boost.MI.
I liked a lot all the extra sections. The "external dependencies" is welcome.
The implementation detail parts provides a very interesting view on some of the rationale for the library. I liked the snippets of conversation with the mentor.
Good old times... :)
* What is your evaluation of the potential usefulness of the library?
Extremely useful. While the simple case of two synchronized maps is not hard to do, it is error prone, and often quick ad-hoc implementations do not have the full generality of boost bimap. Going full MultiIndex is an overkill for simple projects (the interface is definitely more complex).
That is exactly the main reason of the library.
The *_of set types add value to the library but probably are going to be less used.
Options like multiset_of to control the constraints of the bimap will be used a lot IMO. Maybe list_of and vector_of will be very rare.
But before finishing, I have thank again Matias for the help in using boost.build and quickboost during the pas SoC.
You are welcome. The SoC was a really great experience. Thanks for the review! Best regards Matias