On Fri, Nov 18, 2011 at 1:49 PM, Thomas Heller <thom.heller@googlemail.com> 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...
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
Le 18/11/11 23:44, Lorenzo Caminiti a écrit : 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