On Sat, Nov 19, 2011 at 10:13 PM, Edward Diener <eldiener@tropicsoft.com> wrote:
This is my review of the Boost Local library by Lornezo Caminiti.
Hello Edward and thank you very much for your review.
Please state clearly whether you think this library should be accepted as a Boost library.
First I vote for the Boost library to be accepted into Boost.
:)
Other questions you may want to consider:
- What is your evaluation of the design?
I think the design is very logical, easy to understand, and works very orthogonally. The implementer has put much thought into making Boost local easy to use, while retaining some advanced functionality.
I see the local functions having the most importance by far, as an alternative to Boost lambda ( Boost Phoenix ) and C++0x lambda. Personally I do not see myself having much use for local functions but they do provide an alternative arsenal in software design and they do emulate the local function syntax in other languages fairly well. While I personally see little practical use for local functions in software design I would not want to vote against a good C++ implemementation of them, and I have no doubt after reading the documentation and trying out the code that the implementor of Boost local has done an excellent job of providing an analogy to local functions in C++.
I see almost no use for local blocks since C++ already offers the ability to create blocks within a function by just surrounding soem code with a pair of braces ( { } ). I do understand that local blocks offer some binding abilities for variables outside the scope of the block but I do no believe this binding ability offers much functionality although I do understand that if offers orthogonality with local functions.
OK. As I mentioned before, I'm fine with removing local blocks from Boost.Local because they can be _trivially_ implemented using local functions by a user that needs them: int x = 10, y = x; void BOOST_LOCAL_FUNCTION(const capture x, const capture y) { assert( x == y ); } BOOST_LOCAL_FUNCTION_END(block1) block1(); Instead of: int x = 10, y = x; BOOST_LOCAL_BLOCK(const capture x, const capture y) { assert( x == y ); } BOOST_LOCAL_BLOCK_END
It would be nice if local exits and scope exits could come together as a single methodology, but I see no reason for both of them not existing in Boost if they can not. Once again I see little personal use for scope exits/local exits since my viewpoint of design generally precludes such constructs, which I view somewhat as hacks to make up for poor design. But again I admire the way the implementor of Boost local has designed local exits to be orthogonal with local functions and local blocks.
I understand. At this point it looks like that: 1) Local blocks are removed (because trivially implementable using local functions-- see above). 2) Local exits are remove (because void and this_ are added to the existing ScopeExit-- see other thread). Then Boost.Local is renamed Boost.LocalFunction and it remains about its meat which definitely is local functions. I'd be happy with that.
The use of macros may be seen as somewhat unfortunate but the ease of using the macros makes the library easy to use. I do not think that having macros use two possible interfaces in any way reflects badly on the library, but instead applaud the implementor of the library in this respect. The implementor has clearly done an excellent job of making the use of his macros easy regarding their syntax.
- What is your evaluation of the implementation?
I only briefly looked at it but from what I saw it is meticulous and very well done.
- What is your evaluation of the documentation?
The documentation is clear and really excellent.
- What is your evaluation of the potential usefulness of the library?
For me personally, because of my programming style, I see little use for the library. But for others, who want local function syntax and are used to cleanup code in functions Boost local obviously offers a very easy syntax to use. I would still encourage others to look at lambda functions, both in Boost and C++0x, before they look at local functions for solving design issues, but if they do choose local functions as part of the way they want to design some of their code, Boost local offers an excellent solution.
I agree. The tool choice should be left to our library users (which can consider their personal preferences as well as they have clear visibility on the requirements of the problem their are trying to solve). That is also why I present and compare all the alternatives that I know in the Alternatives section: https://svn.boost.org/svn/boost/sandbox/local/libs/local/doc/html/boost_loca...
- Did you try to use the library? With what compiler? Did you have any problems?
I tried with the latest version of gcc and VC++ on Windows and everything just worked.
- How much effort did you put into your evaluation? A glance? A quick reading? In-depth study?
I put a moderate amount of effort reading the docs and trying out the library. I also looked at some of the implementation, particularly the use of macros.
- Are you knowledgeable about the problem domain?
I am knowledgable about the uses of local functions and also the uses of Boost PP, my own VMD library, and C++ macros in general.
Please also consider the following issues and proposals specific to Boost.Local. Your opinion is welcome on any or all of these.
- Boost.Local's local exits provide the same functionality (and then some) as Boost.ScopeExit. Does this duplication of functionality need to be dealt with, and if so, how?
If local exits and scope exits could be combined, that would be fine, but I do not think this is absolutely necessary to accept the library. I never find that having more than one way of doing anything is detrimental as long as each similar implementation is different in some basic respects.
- Lorenzo is proposing to add boost::local::function:: overload to Boost.Function as boost::function::overload. See
https://svn.boost.org/svn/boost/sandbox/local/libs/local/doc/html/boost/loca...
I do not think this should be part of boost::function but possibly part of boost::utility. Simply because one is providing an interface for overloading a set of functors using templates does not mean it belongs as part of boost::function.
- Lorenzo is proposing to add BOOST_IDENTITY_TYPE to boost/utility. See
https://svn.boost.org/svn/boost/sandbox/local/libs/local/doc/html/BOOST_IDEN...
Yes, I think this should be done. If it is not done the name of the macro should be BOOST_LOCAL_IDENTITY_TYPE.
- Likewise, Lorenzo is proposing to add BOOST_IDENTITY_VALUE to boost/utility. See
https://svn.boost.org/svn/boost/sandbox/local/libs/local/doc/html/BOOST_IDEN...
Yes, I think this should be done. If it is not done the name of the macro should be BOOST_LOCAL_IDENTITY_VALUE.
In general all macros in Boost local needs to start with some basic identifying prefix and BOOST_LOCAL_ is the natural prefix for that.
Lastly, please note that Boost.Local has included a copy of the Variadic Macro Data (VMD) library in boost/detail/preprocessor/variadic_macro_data. Since then, VMD has been modified and integrated into Boost.Preprocessor. After the review has concluded, Lorenzo will remove this local copy of the VMD library and replace its usage within Boost.Local with the Boost.Preprocessor variadic extensions.
As the author of VMD I completely agree with this.
Thanks for VMD (and Boost.Preprocessor variadics extensions)-- it was very useful to support the variadics syntax. Thank you very much for your comments. --Lorenzo