Re: [Boost-users] [boost] Boost.Local Review (Nov 10, 2011 to Nov 19, 2011)

Subject: Re: [Boost-users] [boost] Boost.Local Review (Nov 10, 2011 to Nov 19, 2011)
From: Nat Linden (nat_at_[hidden])
Date: 2011-11-18 12:29:07

On Thu, Nov 10, 2011 at 12:52 AM, Jeffrey Lee Hellrung, Jr.
<jeffrey.hellrung_at_[hidden]> wrote:

> The review of Lorenzo Caminiti's proposed Boost.Local library begins tomorrow, ***November 10, 2011***, and ends on ***November 19, 2011***.
>
> Boost.Local implements local functions, local blocks, and local exits for the C++ programming language.  It allows one to define a function at any declarative scope, including function scope; bind variables from the enclosing scope; and pass this function to templated STL-style algorithms.

First comment: when I saw the name Boost.Local, I immediately assumed
it was related somehow to Boost.Locale. That is, the name suggests
that it somehow helps you localize an internationalized application.
The description above came as a surprise to me. Though I can't
remember exactly where, I think there was a suggestion to name this
library something more like "Boost.Scope," which would be more
intuitive. I would like to request that (or similar) name change.

Why does the name matter? Because there are so many Boost libraries
that one's first problem is to determine at a glance whether Boost
even has a library to address one's current problem -- and which
library that might be. Once you've focused on a particular library, of
course you read its documentation to discover the details. But for
most of us, reading through all documentation for all Boost libraries
is simply not practical.

If I were looking for a tactic to pass a nested function to an STL
algorithm, I'm afraid I'd have skimmed right past the name
"Boost.Local" without further investigation.

If the name of a proposed Boost library might possibly be ambiguous,
it seems important to me to try to make it less so.

> Please state clearly whether you think this library should be accepted as a Boost library.

I vote conditional YES. Conditions synopsized at end.

> Other questions you may want to consider:
>
> - What is your evaluation of the design?
> - What is your evaluation of the implementation?
> - What is your evaluation of the documentation?

The documentation is laudably clear.

> - What is your evaluation of the potential usefulness of the library?

I definitely like the ability to construct nested functions to pass to
STL algorithms. Thumbs up for BOOST_LOCAL_FUNCTION_*.

I must admit I'm not fully convinced of the utility of
BOOST_LOCAL_BLOCK, given the existing ability to do this:

    int i = 17;
    {
        const int& locali(i);
        ++locali;  // ERROR
    }

Using BOOST_LOCAL_BLOCK seems like a lot of typing, and the risk of
making the code harder for your colleagues to read, for what it gains.

Having worked with both try/finally (in some languages, that's all you
get) and with RAII, courtesy of stack objects with constructors and
destructors, I much prefer RAII. So for me, BOOST_LOCAL_EXIT is of
dubious utility as well. I recognize the value of being able to
express cleanup immediately following the corresponding acquisition;
but if I'm going to acquire this resource in one place, chances are
I'll end up needing to acquire it in other places too, so I may as
well write the class that logically encapsulates it. That
encapsulation is usually harder to express with try/finally, and I
don't see how BOOST_LOCAL_EXIT would do better.

That said, I recognize that others may differ on this point.

> - Did you try to use the library?  With what compiler?  Did you have any problems?

did not try

> - How much effort did you put into your evaluation?  A glance?  A quick reading?  In-depth study?

I read the online documentation.

> Please also consider the following issues and proposals specific to Boost.Local.  Your opinion is welcome on any or all of these.
>
> - Boost.Local's local exits provide the same functionality (and then some) as Boost.ScopeExit.  Does this duplication of functionality need to be dealt with, and if so, how?

BOOST_LOCAL_EXIT does indeed seem wholly redundant with
Boost.ScopeExit. I don't think both should be equivalently supported
in Boost: how should a coder pick? If there really are semantic
differences between them, then the documentation for each must be very
clear about those distinctions. If there are not, one should be
eliminated. It would make some sense to me to fold Boost.ScopeExit
into this library, perhaps with forwarding headers for backwards
compatibility.

> - Lorenzo is proposing to add boost::local::function::overload to Boost.Function as boost::function::overload.  See https://svn.boost.org/svn/boost/sandbox/local/libs/local/doc/html/boost/local/function/overload.html

YES! overload has no compelling relationship to Boost.Local. It really
does belong in Boost.Function.

> - Lorenzo is proposing to add BOOST_IDENTITY_TYPE to boost/utility.  See https://svn.boost.org/svn/boost/sandbox/local/libs/local/doc/html/BOOST_IDENTITY_TYPE.html

This seems generally useful, yes. In fact it should probably show up
in a Boost FAQ for trying to pass arbitrary types to macros.

> - Likewise, Lorenzo is proposing to add BOOST_IDENTITY_VALUE to boost/utility.  See https://svn.boost.org/svn/boost/sandbox/local/libs/local/doc/html/BOOST_IDENTITY_VALUE.html

This is one point on which the documentation is not clear. For an
expression that evaluates to a runtime value, what's the benefit of
(e.g. from the cited page) BOOST_IDENTITY_VALUE((key_sizeof<int,
double>::value)) vs. the simpler and more obvious extra set of
parentheses: (key_sizeof<int, double>::value) ?

> --------
>
> Lastly, please note that Boost.Local has included a copy of the Variadic Macro Data (VMD) library in boost/detail/preprocessor/variadic_macro_data.  Since then, VMD has been modified and integrated into Boost.Preprocessor.  After the review has concluded, Lorenzo will remove this local copy of the VMD library and replace its usage within Boost.Local with the Boost.Preprocessor variadic extensions.
>
> --------

Conditions:
1. Rename library to something less ambiguous (avoid resonance with
internationalization).
2. Cage match with Boost.ScopeExit.
3. Move 'overload' to Boost.Function. It doesn't belong in Boost.Local.
4. Move BOOST_IDENTITY_TYPE to boost/utility.
5. Use official Boost.Preprocessor VMD support rather than a
library-specific copy.

Comments:
* For me the value of this library is
BOOST_LOCAL_FUNCTION_PARAMS/NAME. It doesn't matter much to me whether
BOOST_LOCAL_EXIT or BOOST_LOCAL_BLOCK come along for the ride.
* I don't really see the point of BOOST_IDENTITY_VALUE either.