Re: [boost] [review] Review of Flyweight library started January 21 and still running!

From: JOAQUIN LOPEZ MU?Z (joaquin_at_[hidden])
Date: 2008-02-03 17:52:38

----- Mensaje original -----
De: "vicente.botet" <vicente.botet_at_[hidden]>
Fecha: Domingo, Febrero 3, 2008 12:00 pm
Asunto: Re: [boost] [review] Review of Flyweight library started 
January 21 and still running!
Para: boost_at_[hidden]

> Hi all,
> 
> Thank you very much Joaquín , I have learn a lot with your library
> and the review. Thanks also for all the pertinent answers.

You're welcome. Thank you for taking the effort to do such a
thorough review.

[...]
> > * What is your evaluation of the design?
> 
> The library has a simple to use interface based on a understandable
> domain-specific language having a good configurability and 
> extensibilitydegree, even if the naming of some policies could  be 
> improved.
> This easy to use interface hides a more complex design. The relations
> between the different policies are not evident, and even if the 
> libraryprovides a elegant way to extend it, it's no evident to 
> define such extensions without reading the code. Maybe the
> documentation should include a deeper description of the
> collaboration of the core implementation and the different policies.

Point taken. Will try to improve that part.

[...]
> * performance test with intermodule_holder
> The tests performed on example5 should be extended with a 
> intermodule_holder specifier.

I don't see the need: remember the intermodule_holder affects
the static initialization process *only*, so the performance
of flyweight<T,intermodule_holder> is exactly the same as that
of fylweight<T>, save for the startup and shutdown phases of
the program.

> * intermodule_holder could use static_holder_class
> As the intermodule_holder is a kind of workaround for platforms
> where DLL can duplicate static libraries, we can have a better 
> performance on the others for free.
> 
> On platforms on which static variables are unique inter DLL the
> intermodule_holder specifier type can be a static_holder_class.
> This could be controlled using conditional compilation.

Yes, this is already taken note of.

[...]
> * holder specifier renaming
> The holder's main responsibility is to instantiate a unique 
> instance for its internal resources depending on the scope
> of the flyweight class.
> 
> So the variation here is the scope. I haven't a better suggestion, 
> but in my opinion holder do not reflects its responsibilities.

If someone comes up with a better name I'll be happy to
change it. I admit the current terminology is not terrific.

> * concrete holder specifiers renaming
> I'd RENAME static_holder by module_holder, and intermodule_holder
> by process_holder.
> module_holder reflects much more the scope accessibility intent 
> for the users, static_holder talks more about the implementation.
> 
> So at the end there will be two holders:
> . module_holder (renaming of static_holder) and
> . process_holder
> 
> I don't think that simple_holder is better than static_holder.
> Does simple_holder stands for simple implemented holder?

Yep, this is the idea. As in other terminology issues, I'll
do whatever people agree upon. The problem is that in my experience
names are something people rarely agree upon :-/

> * factory specifier renaming
> Maybe repository could be more adapted to the associated 
> responsibilities,store the shared values.
> 
> * equality semantic
> It would be great if the library reach to manage with the 
pathological
> cases without reducing the performances of the usual cases.
> If in order to solve the problem you propose the user overrides the
> operator== with non-& semantics this must be included in the 
> documentation.

This is probably what I'll finally do, given the extremely
unlikeliness of the pathological cases.

> * adding a exclusive core specifier.
[...]
> As the interface do not change, this could be for future work.

Yep, thanks for your syntax suggestion, this will definitely
go to the future work section.

> > * What is your evaluation of the implementation?
> 
> Quite good .
> 
> Only some details
> *  Minor correction
> The documentation states that the ..._specifier must be a 
> ..._marker, but
> this is not needed for the ..._class
> On the code static_holder_class inherits from holder_marker, and
> hashed_factory_class inherits from factory_marker.
> Is this an error without importance, or there is something behind?

There is something behind :) As you point out, it's only
specifiers that need be marked as such. But as a bonus extra,
the factory classes provided by the library are also specifiers
on their own. How come? You can do so by using MPL lambda
expressions, for instance, the specifier:

  // use std::greater reather than the default std::less
  set_factory<std::greater<boost::mpl::_2> >

can equivalentely be written like this:

  set_factory_class<
    boost::mpl::_1,boost::mpl::_2,
    std::greater<boost::mpl::_2>
  >

You can take a look at the tests, which exercise both forms
of specification. I decided not to mention that factory
(and holder) classes can act as specifiers because I think
readers not very acquainted with MPL might easily get confused
(understanding lambda expressions in conenction with the
specifiers alone can be already quite a task). This ability used
to be documented at the reference, but I've just checked
and it is no longer there, I'll restore that bit.

> * refcounted_value is a class that could be public. Why it is 
> declared in the namespace detail.

It is an implementation detail, the user won't ever be exposed
to it.

> 
> * could you explain why detail::recursive_lightweight_mutex is 
> needed and boost::detail::lightweight_mutex is not used directly?

Recursive mutexes are needed when constructing composite complexes
with flyweight<>.

[...]
> A more general Boost question. Can boost libraries use the details
> namespace (i.e. private implementation) of the other boost libraries?

No, this shouldn't be done. If some detail of lib A is
deemed interesting for use in lib B, the proper thing to do
is to lift this detail to namespace boost::detail and folder
boost/detail. I've done this in the past with some parts of
Boost.MultiIndex that are now used by other libraries.

> What is needed to promote this shared private implementations to a 
> publicboost library? Should someone purpose the library?

I understand this is what fast track reviews are about.

[...]
> Some more elaborated examples should be included in order to show
> the user how to manage with more complicated examples, for example
> inter-process flyweights. These examples should be a probe of the
> extensibility mechanisms and a check the orthogonality of the 
> policies.

I'll try to extend the examples section. As it happens, this is
one of the tasks I've got most difficulties with, since it's
not easy to come up with good example ideas, and I try to keep
them fun and motivating if possible.

> * intermodule_holder specifier
> A reference to a document explaining the problem with  platforms not
> ensuring the unicity of static variables when DLL are used will be 
> welcome.

I'm no expert here, but I'd say that platforms without
static var unicity across dynamic modules are the majority rather
than the exception.

> * Reference section
> The reference section was much more hard to follow. I think that the
> interactions between the different classes are very hard to describe
> using the current style.

Well, I know the reference is not the easiest thing to follow,
but it tries to keep the level of formality you find at the
C++ standard. The idea is you go to the reference for exact,
authoritative information. Once you get used to the stiff style,
it gets bearable.

> Un implementation section will help to understand how all this works,
> describing the interactions between the core and the policies.

I understand the reader might be curious about the internal
implementation, but is this knowledge really needed to use and
extend the lib? I'd much prefer to concentrate on improving the part
on extending the library without disclosing too much about
how things are assembled internally.

> * Performance section
> I expect a performace section to be included. And how the future
> work  will improbe this performances.

Yes, this will be done.

> 
> * Rational
> Adding of the map versus set problem  explanation.

I've got something prepared to ask Alberto on this one, please
read the following post of mine.

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