[boost] [bimap] Late Review

From: Giovanni Piero Deretta (gpderetta_at_[hidden])
Date: 2007-03-03 17:14:55

Looks like I didn't make it in time.
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.

First of all, I think that the library should be accepted, but that
doesn't matter anyway now :)

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

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

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

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.

The one minute tutorial explains very well the the basic bimap<A, B> interface.

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.

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

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.

The reference section is impressive, with detailed explanation for
every member function. Great job.

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.

* 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).

The *_of set types add value to the library but probably are going to
be less used.

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

I did study the documentation but didn't try the library.

* Are you knowledgeable about the problem domain?

As many, I had the need to maintain two synchronized std::map to do
look up from both sides. I've also used boost.MI. I'm definitely not
an expert on MI internals.

Ok, that's it.

But before finishing, I have thank again Matias for the help in using
boost.build and quickboost during the pas SoC.

gpd