Re: [boost] [local] Review

Subject: Re: [boost] [local] Review
From: Vicente J. Botet Escriba (vicente.botet_at_[hidden])
Date: 2011-11-19 11:59:59

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

Best,
Vicente