On Tue, Nov 15, 2011 at 6:05 AM, Andrzej Krzemienski <akrzemi1@gmail.com> wrote:

Hi Lorenzo, I would like to provide some comments on Boost.Local.

Hi Andrzej, thank you very much for your review (even if there was no yes/no conclusion :) ).

...

Please state clearly whether you think this library should be accepted as a Boost library.

I am unable to give a yes/no answer. This is mostly because I am not clear who is the intended audience of the library. I explain this in detail below.

I am impressed with the library documentation. It is very informative with lots of examples, benchmarks, comparisons, rationale, and layered design (overview, and then details). This is how a Boost library's documentation (and every documentation, IMO) should look like. Yet, one thing that I am missing is what practical problems (that are not already solved) this library is trying to solve. I am not saying that it doesn't solve any. I am just saying I cannot figure it out. A typical story for a Boost library is that there is a common problem that many developers face fairly often, and at some point someone decides to fix this problem comprehensively and ultimately. This is the impression that I had, for example, with Boost.Optional. When I read about it I thought "Yes! This is what I was missing for a long time". I cannot see this in Boost.Local. Perhaps I am not the intended audience of the library, but it would help me if it was clear who the audience is. (I provide examples of what I mean below.)

Yes, Boost.Local vs. C++11 lambdas vs. Boost.Lambda vs. Boost.Phenix vs. etc was subject of long discussions when I asked for interest in Boost.Local: http://lists.boost.org/Archives/boost/2010/08/170138.php This culminated with the Alternatives section: https://svn.boost.org/svn/boost/sandbox/local/libs/local/doc/html/boost_loca... I think in a nutshell the conclusion (which I will detail in the rest of my comments below) was that Boost.Local is handy but on compilers with C++11 lambdas you'd just use lambdas. At the same time, Boost.Local allows you to write local functions that work also on C++03 compilers and that have the same compile-time and run-time performances of lambdas on C++11 compilers. So the /main/ audience would be someone (like myself) that wants to do local function stuff and expects lambda-like performances on C++11 but also has to deal with C++03 compilers. There are other benefits like const& binding which is not supported by C++11 lambdas (so how can I bind by const a variable that has a very expensive copy constructor using C++11 lambdas?) and improve scope exit (i.e., local exit) but these other benefits are probably minor in the general domain (they can be very important or even essential is a specific and narrow domain like Contract Programming but not critical in the general case).

Documentation clearly separates three parts: local functions, local scopes and local exits. Also my impression is different on the three parts. This suggests to me that they could be three separate libraries. What do they have in common? The syntax, I guess. And the fact that you declare them inside functions. But is that enough to bundle them together? Consequently, I divide my comments into three parts.

The reasons why these are together are: 1) They are all about declaring stuff at local scope. 2) Once you have local functions then local exits and local blocks are trivial corollaries to implement (define a local function and call it right away, define a local function can call if from a destructor). I can add this rational in the docs (currently it's not there).

LOCAL FUNCTIONS

Why would I want to use it? Personally, I have one compiler that does support local classes as template parameters. All my other compilers

You might still want to use Boost.Local on this compiler so to: 1) Avoid the boiler-plate code for binding variables in scope (especially if you don't want to repeat their types for example to facilitate maintenance). 2) And, to write code that will work on other C++03 compilers that do not accept local types as template parameters.

support lambdas. So I guess local functions are not for me. So who will use

You'd probably always use lambdas unless you want to write local functions that also work on C++03 compilers (or the pathological case when you must bind my const and you can't copy the variable value).

them? Someone that does not have too new compiler (because they have superior - in terms of performance - alternatives: lambdas, local classes). Someone that does not have a too old compiler, because we read in the documentation that "The implementation of this library uses preprocessor and template metaprogramming (as supported by Boost.Preprocessor<http://www.boost.org/doc/libs/release/libs/preprocessor/doc/index.html>and Boost.MPL <http://www.boost.org/doc/libs/release/libs/mpl/doc/index.html>), templates with partial specializations and function pointers (similarly to Boost.Function<http://www.boost.org/doc/libs/release/doc/html/function.html>). As a consequence, this library is fairly demanding on compilers' compliance with the C++03 <http://www.open-std.org/JTC1/SC22/WG21/docs/standards>standard."

Yes, your compiler needs to be C++03 compliant and maybe old supposedly C++03 compilers were not. Plus support for typeof is needed if you don't want to specify the bound types manually. Your point is correct but I must say that the compilers I have to use at work follow under this category where Boost.Local will be useful (and probably that won't change for another 5 years or so :( ).

Further someone who does not mind compile-time and run-time performance overhead. And someone,who prefers the new syntax to creating a

The overhead goes away on C++11 compilers but yes it is there for C++03 (unless the compiler has the non standard feature that allows local types ad template parameters).

functor class non-locally. I am not sure how many users that  would be?

Yes, it is a very basic trait of the library users that they want to program the function locally without using global functors. IMO, this is a common case and there is no real answer for this without C++11 lambdas.

The documentation mentions proposal N2511<http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2008/n2511.html>, which says that even though with lambdas and local classes we already have a good tool, we still want to have a normal function syntax for local functions. Boost.LocalFunction does not enable a normal function syntax either, so it is no better an alternative than lambdas or local classes.

Yes, I don't buy the N2511 syntax argument neither.

Except for "const binding" which I find hard to appreciate. And except for

I agree (const binding is important/essential only in narrow and specific domains).

compilers that don't support C++11 features, but than again, working only with the new compilers I may not be the right person to review the library.

Is this supposed to provide a seamless migration of the code from compilers w/o lambdas to those that do support lambdas?

Yes: Boost.Local allows to program local functions that compile on both C++03 and C++11, and have the same compile-time and run-time performance as lambdas on C++11 compilers. Is this useful?

I am not comfortable with the syntax: we first specify parameters, then function body and last, we specify the function name. I am not comfortable that the other Lorenzo's library (that I await impatiently) -

:)

Boost.Contract - will provide a different function syntax than Local Functions. This will render the situation where every library will require of me to learn different function declaration syntax.

Boost.Local and Boost.Contract have different domains so their DSELs are different.

On the other hand, the documentation clearly says why this syntax is necessary, so I am not saying "change the syntax".

Yep.

I am suspicious about the local function overloading. While I can imagine writing a local function; writing a bunch of them inside another function would be likely be some error in my design.

BTW, the overload<> template allows to overload any functor, not just local functions.

LOCAL BLOCKS

Again, why would I need them? The example with assertion does not convince me. That is, I face the practical tradeoff: additional enforcement on assertions at the cost of slower build, slower execution time and less readable syntax. I will choose not to use the blocks.

I understand but in some domains (e.g., Contract Programming) this trade off might be different.

The only application that I can think of is a "temporary usage" while refactoring. If I work with the code where functions are 1000-line-long, I would like to divide them into blocks and check which parts reference what variables. In this case, as one of the footnotes in documentation suggests, allowing "return;" inside blocks would be a dangerous thing. But again, if I had a luxury of lambdas, I would choose lambdas for that purpose, because they allow specifying the default binding ( [&], [=] ).

Sure.

It looks like local blocks are only a syntax sugar over local functions. Am I right?

Yes, you define a local function with no result type and no parameters and call it right away. However, the fact that local blocks are a corollary of local functions hat does not mean that local blocks are not useful.

LOCAL EXITS

While exception specifications for locals blocks seam strange, I find allowing exception specifications for local exits an error. Local exits should be subject to the same restrictions as destructors (they are implemented as destructor calls anyways): you must not throw from them. And I expect the library documentation to say it explicitly. I find this "warning" missing from Boost.ScopeExit. So I suggest to disallow (if possible) specifying exceptions in local exit, and adding a section in documentation that warns against potential throws from an exit.

Oops, I missed that! I will add a warning to the docs that to comply with STL exception safety, local exits should never throw.

I do not see anything against having two libraries for the same purpose. This is pretty much the same what we already have in Boost in case of state machines. But since we have two, it is useful to ask why I would choose local exit over scope exit. My answer is:

1. Local Exit allows me to specify an empty parameter list (I cannot do it with ScopeExit). - this is a practical problem I faced 2. I can bind to "this".

Without the rest of Boost.Local, I'd just improve the current Boost.ScopeExit to (1) support empty parameter list and (2) handle this (these both require just some pp tricks).

I do not find const binding a practical advantage. Typically for short functions it is hardly a real problem that I confuse = with ==. And exits would typically be one-liners in my code. On the other hand there are reasons why I would prefer ScopeExit to LocalExit: it spells shorter. In LocalExit I have to type word 'bind' for every single parameter even though it is obvious I need to bind them all (because scope guards do not accept normal parameters). The same argument goes for blocks, obviously. I also like ScopeExit's potential to be implemented with lambdas, which might result in performance benefits, and allow the 'default binding'. On the other hand LocalExit cannot be implemented with a lambda, if not for any other reason, then due to the const binding.

It is really nice that i can bind to variable of the name 'bind'; although I cannot use type 'bind' in local functions.

Yes, a local function parameter cannot have a type named bind. I think this is fine or is there a name different than bind that you'd prefer?

GENERAL THOUGHTS

I am really impressed with the usage of macros. It looks like thanks to Lorenzo we are reaching a new level of preprocessor programming. Who knows

It all starts with Boost.Preprocessor!

how far we can go. I do not think they are overused, given the difficult task. On the other hand, error messages that I encountered give me no clue on what I have typed wrong.

Unfortunately, when using macros there are intrinsic limitations of the errors you can detect and therefore raise intelligibly. However, after a little while that you use Boost.Local, you'll get the function declarations right or be able to find the errors just inspecting the macro calls by eye. More complex errors are usually within the function definition which is programmed outside the macros so compiler errors will help you there as usual.

Decsriptions in the documentation, and the rationale clearly indicates the very detailed knowledge of many C++ aspects.

One suggestion for the future, although I am not sure how useful it would turn out in practice is to have something like local expression, or even not necessarily a local one. This would be a function whose body consists of a single expression. The benefit you get is that the programmer does not need to specify a return type for it (you can use BOOST_TYPEOF for that), or doesn't even have to type keyword 'return', but this may be too much off topic.

Can you clarify with an example?

Term 'C99 <http://www.open-std.org/jtc1/sc22/wg14/www/projects#9899> and later compilers' may be a bit of an overuse for C++ compilers. I suggest 'compilers supporting variadic macros'.

OK.

Also, in reply to particular review questions:

...

What is your evaluation of the design? It is clear. Some macros are difficult to read, e.g. Function arguments first, function name second, but I guess this is a necessity given the domain.

...

What is your evaluation of the implementation? The impressive macro magic is beyond my cognitive capabilities

...

What is your evaluation of the documentation? It is very good. It meets Boost's and my personal criteria for a good documentation.

...

What is your evaluation of the potential usefulness of the library? Are you knowledgeable about the problem domain? This is the part where I have problems. I am not sure about the scope and intended audience of the library.

IMO: The main use is if I need to write local functions which have to perform (compiler and run times) as well as lambda on C++11 compilers and they also have to work on C++03 compilers.

...

Did you try to use the library?  With what compiler?  Did you have any problems? I tried to use the library (basic examples) with VC++8 and VC++10 and Boost 1.42. I found that it does not compile if I use compiler option /ZI (Debug Information Format: Program Database for Edit & Continue) on either compiler.

I will look at why it doesn't compile with /ZI.

...

How much effort did you put into your evaluation?  A glance?  A quick reading?  In-depth study? I spent one evening on reading the documentation and playing with the basic examples.

For the last remark, I believe that the criterion for accepting or rejecting a library is that if it addresses a real-life problems and if it is well designed. Boost.Local is well designed, but the intended audience and primary motivation is not clearly stated, so I find it hard to assess if the first criterion is met.

All in all, Boost.Local is an impressive piece of work.

Thanks a lot for your review! --Lorenzo