Re: [boost] [local] Review

Subject: Re: [boost] [local] Review
From: Lorenzo Caminiti (lorcaminiti_at_[hidden])
Date: 2011-11-19 18:04:59

On Sat, Nov 19, 2011 at 11:59 AM, Vicente J. Botet Escriba
<vicente.botet_at_[hidden]> wrote:
> Le 18/11/11 23:44, Lorenzo Caminiti a écrit :
>>
>> 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.
>>>
>>>
>>> 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.
>
> I agree that it will be better if the documentation used the variadic macro
> forms and introduce the sequence form as a workaround, as I suggested in my
> review.

Yes, I will leave just the variadic macro syntax throughout the docs
and examples while mentioning the sequencing syntax only in an Annex.

>>> Local Functions:
>>>
>>
>>> 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 will accept that Boost.Local doesn't default parameters, as I don't think
> default parameter to be of high value for local functions.

I'd be OK with removing default parameter support from Boost.Local
because they are not a very important feature for local functions.
However, I would personally prefer to leave default parameter there
and if a user doesn't want to use their ", default" syntax, he/she
will simple don't use default parameter (this way if a user really has
to use a default parameter, he/she has the option to do so). I will of
course comply with the final decision from the review.

>>> 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.
>
> I was thinking also about that, but I didn't see how the split could be
> done. Thinking a little bit more on this issue, what about using a keyword
> 'capture/bind' to split them
>
> BOOST_LOCAL_FUNTION(T1 p1, ..., Tn pn, capture, v1, ... vn)
>
> Could this syntax be implemented?

Yes but it does not support binding by const/ref one variable but not
another and specifying the type for the bind (instead of using
typeof). This could be done (using capture, or bind, or any other word
we wish):

BOOST_LOCAL_FUNCTION(T1 p1, ..., Tn pn, capture(v1, &v2, const v3,
const &v4, (int)v5))

I personally don't find this more readable than the existing
Boost.Local syntax (especially when the type is explicitly specified
like for v5). I'll comply with the decision from the review.

>>> 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.
>
> 'capture' seams OK to me. See above.
>
>>> 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).
>>
> Even if this_ is not a placeholder, I will accept that Boost.Local uses
> _this, to be more homogeneous.
>
>>> 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 blocks could be included internally to Boost.Contract as this has no
> general use.

Local blocks are trivial to implement once you have local function so
I'm also OK removing them from the library:

int x = 0;
int y = x;
void BOOST_LOCAL_FUNCTION(const capture& x, const capture& y) {
    assert( x == y );
} BOOST_LOCAL_FUNCTION_END(block1)
block1();

That said I would personally prefer to leave local blocks in the
library so a user has them "out of the box" if he/she needs them. I'll
of course comply with the decision from the review.

>>> 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).
>>
> I will also accept to maintain separated Boost.Local and Boost.ScopedExit
> with the accepted improvements.
> The macros needed in both libraries could be located in a specific detail
> directory.

That sounds good to me.

Thanks a lot.
--Lorenzo