Re: [boost] [local] Review

Subject: Re: [boost] [local] Review
From: Thomas Heller (thom.heller_at_[hidden])
Date: 2011-11-18 20:35:01

On 11/18/2011 04:44 PM, Lorenzo Caminiti wrote:
> 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).

I see, makes sense.

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

Every section has at least one example and the syntax box showing both.
https://svn.boost.org/svn/boost/sandbox/local/libs/local/doc/html/boost_local/Tutorial.html#boost_local.Tutorial.local_functions
Has one paragraph about the differences.
https://svn.boost.org/svn/boost/sandbox/local/libs/local/doc/html/boost_local/Tutorial.html#boost_local.Tutorial.Empty_Parameters
Is basically all about the different syntax possibilities.
The rest only has examples and an optional syntax box showing both.
The corresponding reference sections, obviously have the same.

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

It is not that it is confusing, but why do you need two around? Isn't 
one enough? What about if you decide to change/add something to the 
syntax, you need to update anything else.
 From a user point of view, what are the advantages/disadvantages over 
one or the others?
This is all not clear, if I would have a been a user i would clearly 
choose the sequencing syntax, cause it is portable (over compilers). So, 
why do you need to clutter the docs with having to explain the two syntaxes?

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

It tries do make it easier for users to define local functions in order 
to make their lifes easier so they don't have to clutter their code with 
local functions. This is what i fail to see:
1) The macros do not even closely resemble any kind of function 
declarations in C++
2) If you just want to write a simple local function, the epi- and 
prologue is longer than the actual function body (this is true for 
almost all your examples). And once the function body exceeds that limit 
I wonder why it should have been a local function to begin with.

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

What kind of boilerplate code?
Defining a global function? Most likely no.
Defining a local functor? Yeah, probably, but do we really need that?
Defining a global functor? This is not possible with Local, so the 
answer would be "no" here.

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

Well, I didn't say that I think ScopeExit is good. In fact, I never saw 
the need to use it. As another reviewer noted, i'd rather encapsulate 
that in a class to logically divide that from the actual code.
For ForEach the situation is completely different. Apart from having it 
to spell all caps, it looks exactly like a range based for each loop 
should look like. Boost.Local's Macros (yes, plural!) however, have 
nothing in common with the way you declare functions in C++.

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

See above.

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

Because Local Functions sole API are macros.

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

Right, i have to learn a new syntax in order to learn how to define 
local functions. I note this in order to support the argument that 
Boost.Local does not solve the issues discussed above.

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

This is not something i would expect from a library that should make it 
easy to write local functions.

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

Yes that is what i meant. It is a overloading of terms.

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

Nope it hasn't been documented because there is no real use for that yet.

>
>> 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?
Yeah, this is exactly what i meant.

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

Yes that is correct. Nevertheless I don't find the Boost.Local in any 
way "natural"

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

I agree. This doesn't change the fact, that most of the times when you 
want local functions, you want to have them for only a few lines of 
code. This changes the complexity completely ... because then most of 
the code you have to write for that local function is boilerplate to 
avoid boilerplate. (So i would ask myself ... why didn't i just write 
that global function myself real quick?)

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

See above.

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

Well, at least it doesn't add anything to any of the existing boost 
libraries. While it obviously does a completely different thing than the 
other three mentioned. Sorry to say it so bluntly, doesn't mean that it 
does a good job in doing so. Since you like quoting from these old 
threads. I wasn't a fan of the syntax back then either.

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

Sorry, I just don't get it ... why does it need to be surrounded by a macro?

<snip>

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

I can't fully appreciate the value here. Not because of other existing 
libraries, but because normal functions are way more expressive and 
clear than these macros can ever be.

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

One of the biggest points is certainly the syntax. The other thing is 
that I don't like the idea of advertising the heavy use of macros in 
application developer codes to interface a boost library.