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

Subject: Re: [boost] [flyweight] post-review version available
From: Alberto Ganesh Barbati (AlbertoBarbati_at_[hidden])
Date: 2008-09-11 16:38:07

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

2) It's good to have an optional key extractor

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.

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.

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

HTH,

Ganesh