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