[review] Fast track review of Boost.Utility/Singleton begins today
Hi all, Today starts the formal Fast-Track review of the Boost.Utility/Singleton library. This is a very useful utility, so I'm expecting lots of reviews ;) *Description* A Singleton describes a globally accessible and unique facility, typically implemented as a class with inaccessible constructors and a globally accessible function for clients to access a single instance which is created when accessed for the first time. This library provides three Singleton templates that share an identical interface : |- boost::singleton| : can be used to create Singletons with synchronized initialization. If further synchronization is required it has to be implemented by the user. |- boost::mutexed_singleton| : additionally ensures that concurrent access to the instance is mutually exclusive. In other words, only one thread can access the instance at a given time. |- boost::thread_specific_singleton| : instances exist once per thread. *Author* Tobias Schwinger *Download* Get it from here: http://www.boost-consulting.com/vault/index.php?action=downloadfile&filename=singleton.zip&directory=X-Files Read documentation online here: http://torjo.com/tobias/ What to include in Review Comments ================================== Your comments may be brief or lengthy, but basically the Review Manager needs your evaluation of the library. If you identify problems along the way, please note if they are minor, serious, or showstoppers. Here are some questions you might want to answer in your review: * What is your evaluation of the design? * What is your evaluation of the implementation? * What is your evaluation of the documentation? * What is your evaluation of the potential usefulness of the library? * Did you try to use the library? With what compiler? Did you have any problems? * How much effort did you put into your evaluation? A glance? A quick reading? In-depth study? * Are you knowledgeable about the problem domain? And finally, every review should answer this question: * Do you think the library should be accepted as a Boost library? Be sure to say this explicitly so that your other comments don't obscure your overall opinion. Best, John Torjo - Review Manager - -- http://John.Torjo.com -- C++ expert ... call me only if you want things done right
John Torjo wrote:
Today starts the formal Fast-Track review of the Boost.Utility/Singleton library.
What to include in Review Comments ==================================
Here are some questions you might want to answer in your review:
* What is your evaluation of the design?
Very strong. I like it! I've implemented CRTP Singleton templates a few times before -- highlighting the need for a Boost library. ;-) I'm pleased with the deluxe functionality: - singleton, mutexed_singleton and thread_specific_singleton - bypassing the requirement for friend declarations with the ctor taking boost::restricted - 'instance' as a static data member rather than a static member function - the lease mechanism - control of destruction order My one additional comment on design: people are very used to writing my_singleton::instance()->member. I like the coolness of writing my_singleton::instance->member instead, but I suspect I'll hear some grumbling from colleagues who keep getting reminded by compile errors. I wish it were possible to provide 'instance' as both a static data member and a static member function... But I'd be good with having this become the new convention.
* What is your evaluation of the implementation?
I didn't review the implementation.
* What is your evaluation of the documentation?
Very clear, though not entirely complete, as described below. The organization and style is excellent, and when polished, this documentation will be exemplary. I like the examples. Pointing out the ability to bind() instance and/or lease, and the semantics of each, is great. Also the multiple-inheritance example to graft Singleton behavior onto an existing class. I didn't fully understand the thrust of the example described as: "We might as well have a static facility use the Singleton internally, by using non-public inheritance". A few more words about that example would be useful; maybe a bit of the implementation of static void message(). It appears that the third template parameter SubsystemTag was a recent introduction. The documentation (especially the reference material) doesn't describe it enough, and often fails to mention it entirely. The first paragraph under http://torjo.com/tobias/index.html#boost_utility_singleton._singletons_and_d... seems to want to be a bullet list that got flowed instead. Very little is said about BOOST_SINGLETON_PLACEMENT and BOOST_SINGLETON_PLACEMENT_DECLARATION. The reference material mentions them without any explanation. The example http://torjo.com/tobias/index.html#boost_utility_singleton._singletons_and_d... appears to be inconsistent: shouldn't mylib_tag and library_tag be the same, one way or the other? Also, that example combines three different mechanisms: BOOST_SINGLETON_PLACEMENT, destroy_singletons() and SubsystemTag. More explanation of the role of each (why are we using that mechanism here?), and how they interact -- or remain orthogonal -- would clarify.
* What is your evaluation of the potential usefulness of the library?
Very. "If it didn't exist, we would have to invent it." ;-)
* Did you try to use the library? With what compiler? Did you have any problems?
I didn't try it.
* How much effort did you put into your evaluation? A glance? A quick reading? In-depth study?
I read the documentation and cross-checked a couple of interface points (SubsystemTag) in the source code.
* Are you knowledgeable about the problem domain?
Yes, I've implemented the Singleton pattern a few times now.
And finally, every review should answer this question:
* Do you think the library should be accepted as a Boost library? Be sure to say this explicitly so that your other comments don't obscure your overall opinion.
Yes! Thumbs up! Once the documentation is updated as mentioned above, this will be an excellent and much-needed addition to Boost.
Nat Goodspeed wrote:
My one additional comment on design: people are very used to writing my_singleton::instance()->member. I like the coolness of writing my_singleton::instance->member instead, but I suspect I'll hear some grumbling from colleagues who keep getting reminded by compile errors. I wish it were possible to provide 'instance' as both a static data member and a static member function... But I'd be good with having this become the new convention.
It's actually easy, although I don't think it's a good idea: class instance_t { T* operator->() const; T* operator()() const; }; In Christ, Steven Watanabe
Steven Watanabe wrote:
Nat Goodspeed wrote:
I wish it were possible to provide 'instance' as both a static data member and a static member function...
It's actually easy, although I don't think it's a good idea:
Because providing multiple equivalent ways to invoke the same functionality adds complexity without adding value? Or are there other reasons related to the instance() static member function itself?
AMDG Nat Goodspeed wrote:
Steven Watanabe wrote:
Nat Goodspeed wrote:
I wish it were possible to provide 'instance' as both a static data member and a static member function...
It's actually easy, although I don't think it's a good idea:
Because providing multiple equivalent ways to invoke the same functionality adds complexity without adding value?
Exactly. In Christ, Steven Watanabe
Nat, thanks for your very positive review. Nat Goodspeed wrote:
John Torjo wrote:
Today starts the formal Fast-Track review of the Boost.Utility/Singleton library.
What to include in Review Comments ==================================
Here are some questions you might want to answer in your review:
* What is your evaluation of the design?
Very strong. I like it! I've implemented CRTP Singleton templates a few times before -- highlighting the need for a Boost library. ;-) I'm pleased with the deluxe functionality:
* What is your evaluation of the documentation?
Very clear, though not entirely complete, as described below. The organization and style is excellent, and when polished, this documentation will be exemplary. I like the examples. Pointing out the ability to bind() instance and/or lease, and the semantics of each, is great. Also the multiple-inheritance example to graft Singleton behavior onto an existing class. I didn't fully understand the thrust of the example described as: "We might as well have a static facility use the Singleton internally, by using non-public inheritance". A few more words about that example would be useful; maybe a bit of the implementation of static void message().
I might have kept it so brief since I'm not too sure it's good practice. I think I'll better just remove it.
It appears that the third template parameter SubsystemTag was a recent introduction. The documentation (especially the reference material) doesn't describe it enough, and often fails to mention it entirely.
Yes.
The first paragraph under http://torjo.com/tobias/index.html#boost_utility_singleton._singletons_and_d... seems to want to be a bullet list that got flowed instead.
Yes.
Very little is said about BOOST_SINGLETON_PLACEMENT and BOOST_SINGLETON_PLACEMENT_DECLARATION. The reference material mentions them without any explanation.
That "Singletons and dynamic libraries" section explains their purpose. What exactly is missing? I'm not too fond of specifying what those macros expand to and I don't really want to mention them in the introductory section either, as for me they are kind-of workarounds for the imperfect ABIs we have to deal with.
The example http://torjo.com/tobias/index.html#boost_utility_singleton._singletons_and_d... appears to be inconsistent: shouldn't mylib_tag and library_tag be the same, one way or the other?
They should. My head must had been on vacation revising the docs.
Also, that example combines three different mechanisms: BOOST_SINGLETON_PLACEMENT, destroy_singletons() and SubsystemTag. More explanation of the role of each (why are we using that mechanism here?), and how they interact -- or remain orthogonal -- would clarify.
Yes. Regards, Tobias
Tobias Schwinger wrote:
thanks for your very positive review.
Thanks for a well-designed library!
Very little is said about BOOST_SINGLETON_PLACEMENT and BOOST_SINGLETON_PLACEMENT_DECLARATION. The reference material mentions them without any explanation.
That "Singletons and dynamic libraries" section explains their purpose.
What exactly is missing?
I'm not too fond of specifying what those macros expand to and I don't really want to mention them in the introductory section either, as for me they are kind-of workarounds for the imperfect ABIs we have to deal with.
Agreed: I don't want you to document the expansions. But the entirety of the reference coverage consists of the words "Placement declaration" and "Placement definition," with examples showing how to write them. What effects can I expect? The "Singletons and dynamic libraries" section says: "There must be at most one single instance of a Singleton, even if it is exposed by a dynamic library... Keeping the former can be problematic if the management code gets inlined into the client or if symbols are not being merged automatically. As a solution, this library provides two macros, BOOST_SINGLETON_PLACEMENT and BOOST_SINGLETON_PLACEMENT_DECLARATION to compile the effective code into a specific source file." These macros are a workaround for a problem. The problem appears to be more than one instance of the same singleton class in a given process. How would I know I'm susceptible? What's an example scenario? On a lame platform, two or more dynamic libraries in the process each #include a header for a particular singleton class definition, and contain 'instance' invocations? How would I know my platform is lame? What's the solution: putting BOOST_SINGLETON_PLACEMENT_DECLARATION inside the header class definition, and BOOST_SINGLETON_PLACEMENT inside exactly one of the consumer dynamic libraries? What would happen if both libraries contained BOOST_SINGLETON_PLACEMENT? If I'm writing a dynamic library for use by others, and I use someone else's singleton class containing BOOST_SINGLETON_PLACEMENT_DECLARATION, how would I decide whether to include BOOST_SINGLETON_PLACEMENT in my library? Or should BOOST_SINGLETON_PLACEMENT be used in the main program instead? For me, the documentation doesn't yet answer these two key questions: (1) How would I know I need BOOST_SINGLETON_PLACEMENT? (2) Where should I put it?
Nat Goodspeed wrote:
Tobias Schwinger wrote:
thanks for your very positive review.
Thanks for a well-designed library!
Very little is said about BOOST_SINGLETON_PLACEMENT and BOOST_SINGLETON_PLACEMENT_DECLARATION. The reference material mentions them without any explanation. That "Singletons and dynamic libraries" section explains their purpose.
What exactly is missing?
I'm not too fond of specifying what those macros expand to and I don't really want to mention them in the introductory section either, as for me they are kind-of workarounds for the imperfect ABIs we have to deal with.
Agreed: I don't want you to document the expansions. But the entirety of the reference coverage consists of the words "Placement declaration" and "Placement definition," with examples showing how to write them. What effects can I expect? The "Singletons and dynamic libraries" section says: "There must be at most one single instance of a Singleton, even if it is exposed by a dynamic library... Keeping the former can be problematic if the management code gets inlined into the client or if symbols are not being merged automatically. As a solution, this library provides two macros, BOOST_SINGLETON_PLACEMENT and BOOST_SINGLETON_PLACEMENT_DECLARATION to compile the effective code into a specific source file."
These macros are a workaround for a problem. The problem appears to be more than one instance of the same singleton class in a given process.
That's one symptom.
How would I know I'm susceptible? What's an example scenario? On a lame platform, two or more dynamic libraries in the process each #include a header for a particular singleton class definition, and contain 'instance' invocations? How would I know my platform is lame?
The issue deserves careful investigation even with a "non-lame" ABI in place :-).
What's the solution: putting BOOST_SINGLETON_PLACEMENT_DECLARATION inside the header class definition, and BOOST_SINGLETON_PLACEMENT inside exactly one of the consumer dynamic libraries? What would happen if both libraries contained BOOST_SINGLETON_PLACEMENT? If I'm writing a dynamic library for use by others, and I use someone else's singleton class containing BOOST_SINGLETON_PLACEMENT_DECLARATION, how would I decide whether to include BOOST_SINGLETON_PLACEMENT in my library? Or should BOOST_SINGLETON_PLACEMENT be used in the main program instead?
It should probably always be used if the Singleton is in some way part of a dynamic library.
For me, the documentation doesn't yet answer these two key questions: (1) How would I know I need BOOST_SINGLETON_PLACEMENT? (2) Where should I put it?
OK, I'll address them when I revise the docs. Regards, Tobias
Tobias Schwinger wrote:
For me, the documentation doesn't yet answer these two key questions: (1) How would I know I need BOOST_SINGLETON_PLACEMENT? (2) Where should I put it?
OK, I'll address them when I revise the docs.
Thank you!
participants (4)
-
John Torjo -
Nat Goodspeed -
Steven Watanabe -
Tobias Schwinger