[boost] Boost.Local Review - comments by Andrzej

Subject: [boost] Boost.Local Review - comments by Andrzej
From: Andrzej Krzemienski (akrzemi1_at_[hidden])
Date: 2011-11-15 06:05:09

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

> 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.)

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.

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
support lambdas. So I guess local functions are not for me. So who will use
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."
Further someone who does not mind compile-time and run-time
performance overhead. And someone,who prefers the new syntax to creating a
functor class non-locally. I am not sure how many users that  would be?

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.
Except for "const binding" which I find hard to appreciate. And except for
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?

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.

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

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.

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.

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 ( [&], [=] ).

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

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.

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".

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.

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
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.

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.

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'.

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.

> 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.

> 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.

Regards,
&rzej