Re: [boost] [flyweight] post-review version available

Subject: Re: [boost] [flyweight] post-review version available
From: joaquin_at_[hidden]
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
>>> are:
>>>   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:

http://svn.boost.org/svn/boost/sandbox/flyweight/libs/flyweight/doc/performance.html

> 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
> example:
>
>  class ThirdPartyTextureClass // I can't modify this class
>  {
>  public:
>     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 
not overdesign
things and gather user feedback. Such a "factory functor" would be a 
rather special
beast rather than a regular factory, since it should have to do in-place 
construction
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
> requirement.
>   

For key-value flyweights, Value is *not* required to be assignable or 
copy constructible,
*except* if assignment or construction from value is attempted:

  flyweight<key_value<key_type,value_type> > fw;
  value_type v;
  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, 
precisely because
we're having a key-flyweight value to avoid value copying (among other 
things), but
this is up to the user.

> 5) (probably related with point 4) I don't understand the role of
> copy_value()
>   

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