Re: [boost] [bimap] Late Review

From: Matias Capeletto (matias.capeletto_at_[hidden])
Date: 2007-03-04 01:03:45

Hi Giovanni!

On 3/3/07, Giovanni Piero Deretta <gpderetta_at_[hidden]> 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