Alberto Ganesh Barbati escribió:
JOAQUIN M. LOPEZ MUÑOZ skrev:
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 (http://tinyurl.com/5mnwjp ). 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: http://svn.boost.org/svn/boost/sandbox/flyweight/libs/flyweight/doc/performa...
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 construction of Value, much as boost::in_place_factory does (http://tinyurl.com/4dqzoc ).
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 http://tinyurl.com/create.php ) 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