Re: [boost] Boost.Local Review

Subject: Re: [boost] Boost.Local Review
From: Lorenzo Caminiti (lorcaminiti_at_[hidden])
Date: 2011-11-17 07:15:10

On Thu, Nov 17, 2011 at 4:33 AM, Vicente Botet <vicente.botet_at_[hidden]> wrote:
>
> lcaminiti wrote:
>>
>> On Wed, Nov 16, 2011 at 6:16 PM, Pierre Morcello
>> &lt;pmorcell-cppfrance@&gt; wrote:
>>> Hello Boost community,
>>
>> Hi Pierre. Thank you very much for your review!
>>
>>> Here is my review of the local function library.
>>>
>>> - What is your evaluation of the design?
>>> The design is clear. Several iterations were done already to come to what
>>> it
>>> is today. Of course no one is particularly fond of macros, but a huge
>>> work
>>> was done to reduce the potential problems and on the other hand the
>>> library
>>> can really help a lot.
>>>
>>> - What is your evaluation of the implementation?
>>> I did not get too much into the current implementation. I dug it almost
>>> one
>>> year ago, but there were changes since then. The implementation is
>>> correct
>>> given the features of the library (use of 'this', functor,, name,...). If
>>> there were less features, then some optimisations would have been
>>> possible
>>> (example : not use a virtual function, not use a functor). But the
>>> library
>>
>> One change in the implementation was to use static_cast instead of a
>> virtual function to allow to pass the local class as a template
>> parameter. I did some benchmarking and the static_cast approach had
>> better (somewhat faster) run-time than the virtual function approach
>> with essentially same compilation times.
>> https://svn.boost.org/svn/boost/sandbox/local/libs/local/doc/html/boost_local/Implementation.html#boost_local.Implementation.local_classes_as_template_parameters
>>
>>> would in that case have fewer cases of use. I know how the first verion
>>> of
>>> Lorenzo worked but I did not check the latest.
>>> Given the current features, I don't see how to do even better than what
>>> Lorenzo did. So I am happy with the implementation.
>>>
>>> - What is your evaluation of the documentation?
>>> I really appreciate the detailled documentation. On a 'merchandising
>>> note',
>>> the first page is too big. I think this could 'afraid' some new readers.
>>> Introduction + an 8-line motivating example should be enough to appeal
>>> (or
>>> not) the new reader. I am happy to review a submitted library with a
>>
>> Yes, I will revamp the Introduction section making it shorter and
>> adding a motivation note as suggested by Andrzej in his review I will
>> probably only show local functions in the Introduction example and
>> mention the existence of local blocks and exits for which I will refer
>> the Tutorial section. I will probably start the Tutorial section with
>> the current Introduction section and its example.
>>
>>> complete documentation (that does not always happen).
>>>
>>> - What is your evaluation of the potential usefulness of the library?
>>> This library is filling a few strong needs. Several users including
>>> myself
>>> needed local function and const block. I am lazy when it comes to
>>> functors,
>>> I also find this library is pretty nice to make them quick. C++11 will on
>>> that last part change it , but const block are a must have in my opinion
>>> which are not available in C++11.
>>
>> That is correct. I want to clarify your point for everyone with an
>> example. The only way you can make const blocks using C++11 lambdas is
>> to bind by value:
>>
>> xtype x;
>> [x]() {
>>     assert( x == 0 );
>> }();
>>
>> However, this lambda solution for const block will:
>> 1) Obviously, only work on C++11 (and not on C++03).
>> 2) Not compile at all if xtype is non-copyable.
>> 3) Significantly increase run-time if xtype has an expensive copy
>> constructor.
>>
>> Boost.Local allows instead to bind by const& so no copy is necessary
>> (plus it also works on C++03):
>>
>> xtype x;
>> BOOST_LOCAL_BLOCK(const bind& x) {
>>     assert( x == 0 );
>> } BOOST_LOCAL_BLOCK_END
>>
>> Again, local blocks might not be needed everywhere but if you need
>> them for your application domain, C++11 lambdas do /not/ provide a
>> good solution (while I think Boost.Local does).
>> https://svn.boost.org/svn/boost/sandbox/local/libs/local/doc/html/boost_local/Alternatives.html
>>
>>
>
> I have not used yet C++ lambdas, couldn't the following be used to support
> const binding?
>
> xtype x;
> xtype const& xcr; // const binder
> [xcr]() {
>    assert( xcr == 0 );
> }();

I can't check with a C++11 compiler right now, but my understand is
"yes". Using decltype, you don't even have to repeat the type (to make
maintenance easier). I think the most general way to write that is:

xtype x;
decltype(x) const& const_ref_x = x;
[xcr]() {
    assert( const_ref_x == 0 );
}();

The down sides are:
1) The lambda cannot refer to the bound variable using its original
name x (this will actually look very strange in Boost.Contract if I
had to indicate the user to use some magic names for variables in
constant assertions).
2) The extra type manipulation.

I understand these are not big disadvantages in general but IMO the
following is more clear an concise (I'm probably biased ;) ):

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

I would instead prefer the following if C++11 allowed it:

xtype x;
[const&x]() {
    assert( x == 0 );
}();

But C++11 only allows to qualify the bind with the reference &x and
not with the additional const const&x.

Thanks.
--Lorenzo