Subject: Re: [boost] [flyweight] post-review version available
Date: 2008-09-12 04:59:17
Alberto Ganesh Barbati escribiÃ³:
>> JOAQUIN M. LOPEZ MUÃOZ skrev:
>>> I've tried to address most of the suggestions and comments
>>> raised during the review. The two most important changes
>>> 1. Alberto Barbati's insightful review has led to the
>>> introduction of so-called key-value flyweights
>>> (http://tinyurl.com/5mnwjp ). This type of flyweights is
>>> meant for the situations where constructing the underlying
>>> type is expensive and must be avoided except if absolutely
>>> necessary. I think the design is robust enough and covers
>>> the variants (inner/outer key) that arose during the
>>> discussion with Alberto.
> I had a preliminary look at the key/value stuff of the revised flyweight
> library. I assume that the rest has not undergone severe modifications
> so I concentrated myself on that part.
The other major addition is the section on performance:
> First of all, let me congratulate
> with the good job. It really is a big improvement in addressing a
> scenario I faced (too) many times in the past. Here's my first thoughts
> about the new feature:
> 1) I like the idea of having a flyweight<key_value<K,V>> rather than
> flyweight<K,V>. It is more consistent with the rest of the framework
It's also less work for the lib maintainer and less conceptual load for
the user, I hope, than
having two separate class templates.
> 2) It's good to have an optional key extractor
This is meant to address the inner/outer key dilemma, so that you can
have both approaches.
> 3) You assume that Value is constructible from Key. This initially
> stroke me as a big restriction, but in fact in case you have a class
> that doesn't have a suitable constructor, you can always wrap it. For
> class ThirdPartyTextureClass // I can't modify this class
> ThirdPartyTextureClass(const char* filename, /* other params here */);
> /* ... */
> // can't have flyweight<key_value<std::string, ThirdPartyTextureClass> >
> struct texture_wrap : ThirdPartyTextureClass
> texture_wrap(const std::string& filename)
> : ThirdPartyTextureClass(filename.c_str(), /* ... */)
> // now this is good:
> flyweight<key_value<std::string, texture_wrap> > f;
> I believe this might be mentioned in the docs, if you have time for
> that. Another approach that would make sense is to add a "factory
> functor" argument in addition to the "key extractor". This would help in
> cases where derivation is impossible or would look odd, for example if
> the value type is a basic type or a smart pointer.
Allowing for key->value customization is something I definitely have to
put in my
future work section. I preferred not to include anything yet so as to
things and gather user feedback. Such a "factory functor" would be a
beast rather than a regular factory, since it should have to do in-place
of Value, much as boost::in_place_factory does (http://tinyurl.com/4dqzoc ).
> 4) I don't understand why you need Value to be Assignable. I believe
> it's really important that you could support non-Copiable and
> non-Assignable value types. The main reason for using flyweight is
> precisely to avoid having more copies of the value than the strictly
> necessary. If you are going to copy values, you are failing this
For key-value flyweights, Value is *not* required to be assignable or
*except* if assignment or construction from value is attempted:
flyweight<key_value<key_type,value_type> > fw;
fw=v; // for this to work value_type must be assignable.
(See also http://tinyurl.com/create.php )
So, if you stay away from these expressions the only requisite on Value
is that it must be
constructible from Key. I expect that these expressions will be rare,
we're having a key-flyweight value to avoid value copying (among other
this is up to the user.
> 5) (probably related with point 4) I don't understand the role of
This internal function is only used at the situations described above.
Hope this clarifies
the point on assignability of Value.
Thank you very much for re-reviewing the lib!
JoaquÃn M LÃ³pez MuÃ±oz
TelefÃ³nica, InvestigaciÃ³n y Desarrollo
Boost list run by bdawes at acm.org, gregod at cs.rpi.edu, cpdaniel at pacbell.net, john at johnmaddock.co.uk