Re: [boost] [review] Review of Flyweight library begins today January 7

From: Marcus Lindblom (macke_at_[hidden])
Date: 2008-01-23 10:08:37

Joaquín Mª López Muñoz wrote:
> Hello Marcus, thank you for participating in the review.
> 
> Marcus Lindblom ha escrito:
> 
>> However, I'd like to nit-pick some naming:
>>
>> * The namespace should be boost::flyweight, without the s, since that's
>> what the lib & design pattern is called.
> 
> There is a reason for that spurious 's', namely that some compilers (VC6.0
> --though this is probably of little relevance in 2008-- and some versions of
> GCC) have problems with using declarations where the namespace and
> class names are identical. This issue was raised originally in the context of
> Boost.Tuple, and indeed the namespace for that lib is boost::tuples: look up
> the section "For those who are really interested in namespaces"at
> http://tinyurl.com/ywb9n7 for more info.

Ok. Fair enough. :)

>> * The factory not only creates objects, it also stores them. When I see
>> 'Factory' I think creation only, not storage. How about naming it the
>> 'store' instead, since it seems to be the primary function?
> 
> The main reason why the name "Factory" was picked is because most
> descriptions of the flyweight design pattern use this same terminology:
> http://tinyurl.com/ys5k3q . If there's an agreement that the name should be
> replaced by something more appropriate, I have no objection to do so, of
> course.

Ok.

>> Does the factory even create objects? It doesn't seem like that. It just
>> stores copies of them in Entrys?
> 
> Yep, factories stores copies of externally constructed values. In "classical"
> renditions of the pattern, the factory accepts some kind of key to get an
> associated flyweight object: here, the key is simply the value itself.

Ok. I confused this factory with the Factory pattern.,

>>> * What is your evaluation of the implementation?
>> Not looked at thoroughly.
>>
>> However, there is a dependency on boost::hash<>. Could this be changed
>> into conforming to a concept that just allows the value to be converted
>> to size_t?
> 
> I'm not getting your suggestion. Do you mean that flyweight<T> should
> require that T be convertible to size_t? I don't know how this is better than
> relying on boost::hash<>.
 >
> On the other hand, boost::hash<> is merely the default for providing the
> hashing of T. If you have an alternative you can use it like this:
> 
>   struct my_hash
>   {
>     std::size_t operator()(const T& x)const{...}
>   };
> 
>   typedef flyweight<T,hashed_factory<my_hash> > fw_t;

Ok. I misunderstood things a bit and was a fraid that I could not use 
the default hash implementaitons and similar things.

>>> * What is your evaluation of the documentation?
>> Great!
>>
>> However, on the reference page, the first header ("boost/flyweight.hpp")
>> doesn't link to anything?
> 
> Do you mean the second bullet in the "Contents" section? If so, it links to
> somewhere below down the same page, maybe that's why it seemed
> to you to point nowhere. Or are you referring to some other link?

That one, and the first link under "header summary".

I retested, and it works in IE, but not in FireFox.

So, no more worries then. Best of luck with getting the library accepted!

Cheers
/Marcus