Re: [boost] [local] Review

Subject: Re: [boost] [local] Review
From: Lorenzo Caminiti (lorcaminiti_at_[hidden])
Date: 2011-11-18 17:44:38

On Fri, Nov 18, 2011 at 1:49 PM, Thomas Heller
<thom.heller_at_[hidden]> wrote:
> Hi all,

Hello Thomas and thank you very much for your review.

> 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

Why is that? From a user prospective, you just pick a syntax based on
what your compiler can do, write your programs in such a syntax, and
maintain that one syntax. From a library implementation prospective,
supporting both syntax is trivial. There is a macro that converts
variadic into a sequence up front and all the library is programmed
using pp sequences (which are a better pp data structure than
variadics as explained by Paul many times). So as a library maintainer
you only see pp sequences after the 1st macro expansion and you just
have to maintain pp-sequences-- no pp code is duplicated at all to
support both syntaxes. Pseudo code:

// VARIADIC_TO_SEQ(...): converts its argument from variadics to
pp-seq (or it leaves it unchanged if it is already a seq).

#if VARIADIC

#deifne LOCAL_FUNCTION_PARAMS(...)
    LOCAL_AUX_FUNCTION_PARAMS_SEQ(VARIADIC_TO_SEQ(__VA_ARGS__))

#else

#define LOCAL_FUNCTION_PARAMS(seq)
    LOCAL_AUX_FUNCTION_PARAMS_SEQ(seq)

#endif

Therefore, I'd conclude that there is no added maintenance cost or
even implementation complexity in order to support both syntaxes. I
can add this rationale to the Implementation section (currently it is
no there so it might be natural to think that having both syntaxes
comes to a high implementation cost even if it does not).

> nightmare. Also, reading through the documentation, most of the time is
> spent on explaining, the difference between the two syntaxes. Why not just

Which part of the documentation are you referring too? After both
syntaxes are introduced:

https://svn.boost.org/svn/boost/sandbox/local/libs/local/doc/html/index.html#boost_local.Introduction.two_syntaxes

The rest of the docs just explain the functionality. Even if I provide
all examples in both syntaxes, I don't think it distracts the user--
do you? Can you point out the part of the documentation that are
confusing for the user because of the "double" syntax? (Maybe I can
improve them.)

> stick with one and concentrate on the functionality?

Because having the sequencing syntax comes at no maintenance or
implementation cost and it allows to use the library on compilers
without variadic support.

> 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

Why Boost.Local does not solve this appropriately? What do you mean by
"appropriately" in this context?

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

I understand. I disagree but I understand your point. I disagree
because the use of macros in this context saves the user from writing
verbose boiler-plate code.

IMO, it is similar to Boost.ScopeExit: Instead of providing the
SCOPE_EXIT macros we could have said that the user writes the code to
bind the variables deducing their types and it programs the local
class with the exit code in the desstructor all of that by hand.
However, using macros saves the user to write all of that and as I
user I personally appreciate that. It is true that there is a trade
off between the readability cost of using macros and the benefit of
not having to write boiler-plate code. I find that trade off
acceptable for libraries like Boost.SopeExit or even ForEach and (of
course ;) ) also for my library so I don't have to write the
boiler-plate code for the binding and the casting to pass the local
class as a template parameter.

-- from: http://lists.boost.org/Archives/boost/2011/02/176598.php --
On Thu, Feb 3, 2011 at 11:48 PM, Joel de Guzman
<joel_at_[hidden]> wrote:
> On 2/4/2011 12:29 PM, Gregory Crosswhite wrote:
>>
>> Local should be viewed as being complimentary to Phoenix rather competing
>> with it, and it
>> should be evaluated on its own merits.
>
> I think I have to agree with this. In as much as we have Boost-FOREACH
> in addition to std::for_each, Boost.Local is a good and useful library
> that is simple, practical and effective enough as it is.
-- end --

-- from: http://lists.boost.org/Archives/boost/2011/02/176552.php --
On Thu, Feb 3, 2011 at 10:45 AM, Thomas Heller
<thom.heller_at_[hidden]> wrote:
> http://svn.boost.org/svn/boost/sandbox/SOC/2010/phoenix3/libs/phoenix/doc/html/phoenix/inside/extending_actors.html
> A boilerplate macro for that would be a very welcome contribution...
-- end --

So this boiler-plate-saving macros are a commonly accepted practice,
why wouldn't they be appropriate for local functions?

> 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 = [...](...){...}

I meant that this is a limitation of Boost.Local which cannot be used
within expression (and not a limitation of C++11 lambdas). I will try
to make this text more clear in the docs.

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

Others have made the same comments (with you a total of 3 people).
However, others have made the opposite comment (2 liked default, 5+
didn't say anything about it). In my experience, this quickly becomes
a non-issue after the 1st week that you start using the library.

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

You are the first one to make this comment. I think that if you follow
the convention to list the function parameters first and the binds
last, you can easily see the function arity (at least that is my
experience). The library does not enforce this order, but that is the
convention I use in the docs. With that you look at the parameters
from left to right until you find an parameter that said `bind` and
there it is the function arity.

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

I don't think so... here bind is clearly used in a different context.

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

Yes, I will add that to the docs ad suggested by Vicente. This has the
down side of (1) requiring to use a different name for the constant
variabe `icr` and (2) write the extra code to declare the const
variable. I agree that these are both minor points (but in some
specific domains like Contract Programming however (1) is a big issue
because you will have to explain the user that they need to use the
magic name `icr` to refer to the variable in programming their
assertions).

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

Yes, I will always use this_ (because it's a keyword in the context
where it is used in Boost.Local).

BTW, I understand that Phoenix uses _this, do you have a reference to
the docs that discuss the use of _this? (I looked for it but I
couldn't find it... I probably looked in the wrong place.)

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

What defect? I can fix the closure of values. The closure of reference
suffers of the same limitations of C++11 lambdas and that's the
accepted standard definition of closure for a stack based language
like C++.
http://en.wikipedia.org/wiki/Closure_(computer_science)#Implementation_and_theory

Can you please clarify this 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

The function declaration is trivial and it is very easy to get it
right (just inspecting it). If you make an error and you need the
compiler's help to understand what you did wrong that is usually in
the function definition. The function definition is outside the macros
so you will get the usual compiler error support in programming the
function correctly.

I do find this true in my personal programming experience and others
have made the same comment:
-- from: http://lists.boost.org/Archives/boost/2011/02/176517.php --
On Wed, Feb 2, 2011 at 5:12 PM, Mathias Gaunard
<mathias.gaunard_at_[hidden]> wrote:
> On 02/02/2011 13:23, Thomas Heller wrote:
>
>> To quote the Boost.Local docs:
>> "Warning
>> Unfortunately, there are intrinsic limitations to the amount of syntactic
>> error checking that can be done by the parsing macros implemented using
>> the
>> preprocessor. As a consequence, an error in using the parenthesized syntax
>> might result in cryptic preprocessor errors. The best way to identify and
>> correct these errors is usually to visually inspect the signature
>> comparing
>> it with the parenthesized syntax grammar (see the Grammar section). When
>> syntactic errors can be detected by the parsing macros, they are raised at
>> compile-time using error messages of the form ERROR_description_text."
>
> Enumeration of parameters and bindings is not where code complexity is.
--end quote--

> 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

Which points are not met?

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

I don't see how Boost.Local prevents improving other Boost libraries--
a part from the fact that I did spend time in developing Boost.Local
instead of improving the other libraries :).

In fact we all concluded that Boost.Local is complimentary to
Boost.Phoenix, Boost.Lambda, and Boost.Bind in that it provided
statement syntax for the function definition which these other
existing Boost library do not (they provide expression syntax for the
function definition):
http://lists.boost.org/Archives/boost/2011/02/176453.php
http://lists.boost.org/Archives/boost/2011/02/176589.php
To be honest, if we didn't conclude upfront on the complementarity of
Boost.Local with respect to other existing Boost libraries, I wouldn't
even spend the time for the submission-- why would I submit a
redundant library to Boost?

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

I am sure there are lots of people that will never use a local blocks.
That said, I personally needed to program (to ensure the
constant-correctness requirement of assertions in Contract
Programming):

int x = 0;
const {
    assert( x == 0 );
}

You can do that with local blocks:

int x = 0;
BOOST_LOCAL_BLOCK(const bind x) {
    assert( x == 0 );
} BOOST_LOCAL_BLOCK_END

As we said, using lambdas or other Boost libraries there are other
ways to do that but in my case I needed to keep the `assertion( x == 0
)`;` unchanged (including the variable name `x`) and these other ways
(including C++11 lambdas) don't allow for that.

More in general, local blocks allow you to specify the sematic that a
variable should be thread as const by the next bunch of instruction. I
personally find that useful, others have made the same comment
(Vicente, Pierre, Jeff, and another couple of people-- see their
review to Boost.Local).

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

I agree. The ScopeExit improvements alone are not sufficient to
motivate Boost.Local (I'd just add const bindings and this binding to
ScopeExit for that). Boost.Local is about local functions, the
improved scope exits (i.e., local exits) should be somehow merged with
the existing ScopeExit (in fact that's a question to the reviewers).

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

I can't double check now but I am pretty sure was the one from Boost
1.47 (I will add this information to the Alternatives section). As I
mentioned in the Alternatives section, compilation-time was not an
issue for me after I started to #include files selectively (which is
very reasonable) also I was very impressed by the short run-times when
compiler optimization is enabled (good job!).

> 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

What do you mean by "modern C++"?

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

I disagree. Boost.Local adds value because it allows you to use
statement syntax to program the function body (and that is why
Boost.Local is complimentary to these other Boost libraries):
https://svn.boost.org/svn/boost/sandbox/local/libs/local/doc/html/boost_local/Alternatives.html#boost_local.Alternatives.local_functions

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

Given that you have made several comments (which I sincerely
appreciate), could you please summarize what are the main reasons for
voting "no"?

Thanks a lot for your review!
--Lorenzo