Re: [boost] [flyweight] Review period extended to February 3

From: Markus Werle (numerical.simulation_at_[hidden])
Date: 2008-02-03 17:13:49

Thanks for extending this period!

Ion Gaztañaga <igaztanaga <at> gmail.com> writes:

> * What is your evaluation of the design?
> * What is your evaluation of the implementation?
> * What is your evaluation of the documentation?

I only took a short glance at the docs and at the issue page

> * What is your evaluation of the potential usefulness of the library?

I think this lib _could_ become useful for me some day.
No idea yet, but it is nice to know the solution to problems
that I do not have - yet. This is how I use boost all the time ...

> * Did you try to use the library? With what compiler?
>      Did you have any problems?

no try

> * How much effort did you put into your evaluation?

A quick reading

> * Are you knowledgeable about the problem domain?
no

> And finally, every review should answer this question:
> 
> * Do you think the library should be accepted as a Boost library?
>      Be sure to say this explicitly so that your other comments
>      don't obscure your overall opinion.
> 

I think it should be accepted. 

My remarks to the current discusion:

Equality semantics: 
This is where I'd like to see the library changed.
I did not think about this in depth, but it let me feel 
uncomfortable immediately after reading that passage in the docs.

Flyweight is advertized as "drop-in replacement for const T."
and can be used everywhere a "T const &" is expected
and as such it should behave also for operator==. 
I found it odd, that something that is "not a pointer" must 
be dereferenced to be compared.
This breaks too much code for a true drop-in replacement.
Also I think that intuitive behaviour is a GoodThing(TM) for a C++
library. We get so used to all these little oddities that make
you look into the book even after years in C++, but I think
it's complicated enough now. 

The constant time comparison is nice, but it is due to the
IMHO counter-intuitive basic interface (though I can see its advantages). 
In any case I'd prefer comparison of the underlying values,
even for flyweights from different factories (!), just in order
to keep the "drop-in replacement for const T." semantics everywhere
and in any case. 
If I need a safe access that ensures both fws come from the
same factory, and const compare times, etc, I'd prefer a special free 
function compare(...) which then has all the semantics
operator== has now. 

Alternative/simpler configuration interface:

If you use multiindex you get used to the 10-liner-typedefs
here and there in your code, but of course they look ugly.
Same with this library here. OTOH together with the high quality 
of the docs (they are perfect, thanks Joaquín!)
this might not be a problem.

If there is an elegant solution that does make the code 
simpler I'd appreciate its inclusion to flyweight, but it's
not a must-have-before-acceptance.

best regards,

Markus