[boost] [local] Review

Subject: [boost] [local] Review
From: Thomas Heller (thom.heller_at_[hidden])
Date: 2011-11-18 13:49:58

Hi all,

Following is my Review of the upcoming Boost.Local library. I will as 
others split my review into the three parts that make up this library:
local functions, local blocks, scoped exit.

Let me first start with some general comments.
I personally find the decision to support two syntaxes not a good path 
to follow. While I understand the rationale behind it, and the desire to 
have the variadic macro version, having both might become a maintenance 
nightmare. Also, reading through the documentation, most of the time is 
spent on explaining, the difference between the two syntaxes. Why not 
just stick with one and concentrate on the functionality?
Additionally, despite the introductory motivating statement ("Local 
functions are a form of information hiding and are useful for dividing 
procedural tasks into subtasks which are only meaningful locally, 
avoiding cluttering other parts of the program with functions, 
variables, etc unrelated to those parts.") I can not see that this 
library solves this appropriately. I personally think that exposing 
macros as the interface to program with the library does not lead to a 
nice and clean design of ones code.
Boost.Local is very different from other heavily macro based libraries 
like Boost.Parameter. It is different, because I would consider 
Boost.Local not as a infrastructure library but as a library for end 
users. This being said, i would not like to see application code 
cluttered with overly verbose calls to macros.
As boost is trying to not only push the boundaries of C++ it should also 
try to set examples of best C++ practice. I would not consider Macro 
calls to interface with a library not good practice.

Local Functions:

 From the tutorial section on Local Functions:
"Local functions are defined using the following macros (see the 
Reference section) from within a declarative context (this is a 
limitation with respect to C++11 lambda functions which can instead be 
declared within expressions):"

Right, to have the lambdas in a "declarative context" use
auto f = [...](...){...}

The default parameter syntax is confusing, it looks like it is a 
completely new parameter, and not belonging to the previous argument. 
This gets confusing once you have more than one parameter with a default 
parameter. The WITH_DEFAULT macro should be the default, as it clearly 
states the relationship of the default value to the parameter.

I would consider binding variables from the enclosing scope one of the 
major weak points of Boost.Local. The need to binding those variables is 
obvious. However, how it is done clutters the arguments list of the 
BOOST_LOCAL_FUNCTION_PARAMS to an extend that it is not immediately 
clear what the arity of the local function is. It would make things 
clearer to have something like:
BOOST_LOCAL_FUNCTION_PARAMS(...)
BOOST_LOCAL_FUNCTION_CAPTURE(...)

Additionally as binding is probably the right term to use here, it might 
confuse people with the already existing solutions of the various bind 
implementations.

WRT to your note in the documentation that C++11 can not bind const 
reference. I suggest the following workaround:

int i = 0;
auto f = [&](){ auto const & icr = i; ... };

I agree that this and _this should be used always, instead of having 
both _this and this.

On other notes, Lorenzo already clarified the defect in the library that 
it can't really be used as a closure. This would have been my next point.

All in all the documentation is very accurate and in good shape. 
Additionally i played around with this specific part of the library a 
little bit in order to see how i can break it. TBH, I didn't (and still 
don't) trust the macros to do the right thing always, my knowledge of 
the PP isn't good enough to judge that particular part of the 
implementation. However, by deliberately trying to break the breaks 
(putting typos in the variables, omitting commas etc.) the error 
messages are of course very arcane, and you have to know exactly where 
and what part of the macros involved failed and why. Of course, these 
error message can be deciphered very easily once you have enough 
experience with that library. But i would argue that this is the case 
for any other (TMP based) library.
Which brings me to my next point. I think that Boost.Local does not 
fulfill its own purpose. While it is of course true that errors in the 
function bodies are written in "usual C++" the surrounding macros are, 
IMHO, just cluttering the code. The points Lorenzo makes in the 
motivational section are not met. I would argue that most of the times, 
namespaces and classes are good enough, and such a macro syntax should 
be avoided. For everything else, there already exist a couple of 
solutions (Boost.Bind, Boost.Lambda and Boost.Phoenix). I would like to 
see improvements in ease of use in these libraries instead of 
advertising a completely macro based solutions.

Local Blocks:
I can't see the value in that one. The example in the documentation 
doesn't help either, as those things are easily detected by any modern 
compiler, with the appropriate warning levels (ok, not in the assert 
case, but for other boolean contexts at least gcc does).

Local Exit:
Why do we need two libraries doing the exact same thing with neither of 
the adding any functionality. I suggest to improve ScopedExit instead.

In the alternatives section, which version of Phoenix do you use?

Now to the usual questions:
 > - What is your evaluation of the design?

I think the design is not what can be considered Modern C++. However, in 
the context of what the author tried to achieve, he did a good job.

 > - What is your evaluation of the implementation?

The implementation looks solid!

 > - What is your evaluation of the documentation?

The documentation is very good!

 > - What is your evaluation of the potential usefulness of the library?

The potential usefulness of the library is out of question, that is why 
C++11 now has lambdas, and libraries like Bind, Lambda and Phoenix 
exist. However, I do not believe that Local adds any value to the 
existing solution of the described problems.

 > - Did you try to use the library?  With what compiler?  Did you have 
 >   any problems?

Yes. gcc 4.6.2. no problems.

 > - How much effort did you put into your evaluation?  A glance?  A   > 
   quick reading?  In-depth study?

Reading of the documentation. Looking briefly at the implementation.

 > - Are you knowledgeable about the problem domain?

Yes. I am the author of Phoenix V3.

I vote to not include the library into boost.

Regards,
Thomas