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

From: Alberto Ganesh Barbati (AlbertoBarbati_at_[hidden])
Date: 2008-02-01 18:37:02

Ion Gaztañaga ha scritto:
> 
> * What is your evaluation of the design?

The overall design looks very good. However, IMHO it doesn't fully grasp 
the essence of the flyweight pattern because of a major flaw. The flaw 
is the fact that an object needs to be created in order to check whether 
an equivalent flyweight exists or not. Consider this code:

   typedef boost::flyweight<std::string> fw;

   fw s1("foo");
   fw s2("foo");

the creation of s1 requires one call to the string constructor, two 
calls to the string copy constructor and two calls to the string 
destructor. If we could use rvalue references and perfect forwarding, we 
could avoid one or maybe even two of the three temporaries. However 
insertion of a new flyweight should occur rarely in typical use cases, 
so it doesn't matter much. The problem is when constructing s2, a case 
which is going to occur much more often. To construct s2 the current 
implementation calls the string constructor once, the string copy 
constructor once and the string destructor twice. Even in this case, we 
may in the future avoid one temporary, but not the other. We are 
actually creating a full-blown object only to throw it away! That is a 
waste which can be get worse if instead of one std::string you had an 
UDT which is expensive to construct. Notice that the latter is precisely 
the scenario where the flyweight pattern is most useful!

The GoF book describes a flyweight pattern where each flyweight can be 
identified through some key object that is supposed to be cheaper to 
create and manage than the final object. In other words, the GoF pattern 
acts like an std::map, whereas this proposal assumes that the final 
object acts as the key, much like an std::set. I agree that the proposed 
approach may be simpler to explain and to work with and also provide an 
almost drop-in replacement of the original type, but IMHO the map 
approach is far more useful, especially for "heavy" types and for 
polymorphic types (whose construction usually requires a potentially 
expensive dynamic allocation).

> * What is your evaluation of the implementation?

The implementation seems well done. The use of the "named parameters" 
idiom is very interesting.

However, I strongly disagree with the choice of the term "factory" for a 
component that actually only acts as a storage. In GoF the term factory 
is properly used for a component which is devoted to construct the final 
object given its identifying key. But in this proposal it's the user 
code that actually constructs the object, so the term is used 
incorrectly and is misleading.

> * What is your evaluation of the documentation?

The documentation looks very well written, with lot of examples. The 
only sections that I found a bit lacking are the ones about the 
policies. The policies are indeed properly described, but the bare 
description is not very helpful in showing how to write a new policy. In 
particular I couldn't understand how to write a Tracking policy until I 
actually saw the code of flyweights::refcounted.

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

The library as it is, is not very useful, although it might be 
potentially very useful if the flaw I described before could be addressed.

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

Yes, I used it with gcc 3.4.5 (mingw).

>      Did you have any problems?

No, once I realized that you needed Boost 1.35 even if you don't use any 
feature related with Boost.Interprocess.

> * How much effort did you put into your evaluation?
>      A glance? A quick reading? In-depth study?

A couple of hours.

> * Are you knowledgeable about the problem domain?

Yes.

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

Reluctantly, the answer is no. I believe the library should not be 
accepted in Boost as it is. However, it's apparent that great and good 
work has been put into it and it has a great potential. Therefore I 
encourage the maintainer to consider addressing the issues I raised 
here, in which case I am ready to change my opinion.

Ganesh