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

From: Alberto Ganesh Barbati (AlbertoBarbati_at_[hidden])
Date: 2008-02-06 18:17:49

Joaquín Mª López Muñoz ha scritto:
> Alberto Ganesh Barbati ha escrito:
> 
>>
>> 1) we have two types: key_type for keys and value_type for values. The
>> two types can be coincident (as in your non-keyed proposal) or be
>> different (keyed case)
>>
>> 2) the constructor of flyweight semantically takes keys, not values
> 
> OK so far.
> 
> 
>> 3) keys (not values) are fed as-is to the factory insert() method:
>> here's the key point, it's up to the factory to determine whether the
>> key is already present and if it's not present to construct the
>> corresponding value. There's actually no need for a find() method.
> 
> find() would still be useful in combination with rw locks.
> 

If factories are kept "dumb" as they are, yes, it might be useful. See 
below for another possible approach.

> 
>> The flyweight template need not know how to compare keys with values,
>> actually it does not even need to know that it is happening.
> 
> How do I know if value_type is convertible to key_type? I need asistance
> from the user.
> 

The user might be allowed to provide that as an additional extension 
point. However, calling the value_type constructor with the key_type as 
parameter is a reasonable default behaviour.

>> Let me say it again with different words: the flyweight/factory
>> interface has only one method, namely insert() as in the current design.
>> A possible map-base factory might implement the factory::insert() method
>> as a double call map::find() and map::insert(), this would just be an
>> implementation detail of the factory, the flyweight machinery needs not
>> be aware of that.
>>
>> 4) the insert() method returns a handle
>>
>> 5) the factory provides a way to extract values from handles
>>
>> The rest of the design would more or less stays exactly the same.
> 
> Well, this is a valid approach, but I'd rather factories remained dumber so
> that all this intelligence is implemented in the core, because of the following
> reason (among others): STL associative containers (which are the natural
> components upon which to implement factories) are either set-like
> (key_type=value_type) or map-like (key_type is a fixed part of value_type,
> which does not coincide with user's provided T). To be able to accept both
> the keyed and non-keyed variants, the container would have to deal with a more
> general notion of key extraction. Boost.MultiIndex can do that, but I don't
> want to tie flyweight to that lib.

If you stick to that, you really should consider a name different from 
factories, which indeed are nothing more than storage. Anyway, I get 
your point. The design I have in mind is a bit different, though, 
leaving more intelligence to the factory. Even in your design, std::set 
can't be used directly, it's been wrapped in set_factory<>. So what's 
wrong in having this added intelligence put inside the wrapper, rather 
than into the core?

About the "third" case, you don't need to tie Boost.MultiIndex with the 
core. The library would simply propose (among several others) one 
factory implemented as a wrapper onto Boost.MultiIndex. Only user that 
needs that feature would need to Boost.MultiIndex. This would be no 
different than the current design, where hashed_factory depends on 
Boost.MultiIndex and let's not forget that hashed_factory is the default 
factory!

> On the other hand, I don't know how to solve the third case (T convertible
> to K) with dumb factories :-( Maybe this third case is not worth implementing,
> given that the external key variant shouldn't require that much extra storage
> (shared values ought to be comparatively few with respect to the number
> of flyweight objects pointing to them.)
> 
> Maybe I can provide an external flyweight wrapping both behaviors
> 
>   flyweight<T,...> // classical flyweight
>   flyweight<T,key<K>,...> keyed flyweight
> 
> or something to that effect. I definitely have to think this carefully.
> 

If I could rethink the design from scratch (and I know I can't as the 
library has already been accepted) I would probably come up with 
something like:

1) the core class flyweight with has only two parameters: the factory 
and the holder policy

2) a bunch of different factories implementations. In particular there 
should be at least three factories to cover the three basic cases: K==T, 
K!=T non-intrusive and K!=T intrusive. These factories may be 
parametrized with a user-provided container or a separate parametrizable 
factory should be provided ad hoc. Most important, all library-provided 
factories shall be parametrized by the locking and tracking policy

3) a bunch of holder, locking, tracking policy implementations

The choice to have locking and tracking parametrize the factories 
instead of the core is that actually no locking nor tracking happens in 
the core! Locking happens only in the execution of insert() and erase(), 
which would be moved in the factory, and tracking can be obtained as a 
RAII wrapper provided by the factory (I admit that I didn't thought 
about tracking deeply enough to pretend that this is actually possible, 
but I'm sure about locking).

Just my opinion,

Ganesh