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

From: Joaquín Mª López Muñoz (joaquin_at_[hidden])
Date: 2008-01-23 08:42:55

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.

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

> 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.
It is likely that future extensions of the concept will allow for inplace
construction of the values using perfect forwarding capabilities and
variadic templates of C++0x. Along this line, look for "emplace" in the
latest C++0x draft:

http://open-std.org/jtc1/sc22/wg21/docs/papers/2007/n2461.pdf

[...]

> > * 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;

> > * 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?

> Also, the flyweight.hpp page misses to mention the tag argument in the
> first list of parametrization (above the code-block).

Oh, you're right. I'll fix that.

[...]

> > * Do you think the library should be accepted as a Boost library?
>
> Yup! But some names ought to be changed to avoid confusion.

Best,

Joaquín M López Muñoz
Telefónica, Investigación y Desarrollo