Boost logo

Boost :

Subject: Re: [boost] [flyweight] post-review version available
From: joaquin_at_[hidden]
Date: 2008-09-12 04:59:17

Alberto Ganesh Barbati escribió:
>>> 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
>>> ( ). 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
> 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
of Value, much as boost::in_place_factory does ( ).

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

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

Boost list run by bdawes at, gregod at, cpdaniel at, john at