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

Subject: Re: [boost] Boost.Local Review (Nov 10, 2011 to Nov 19, 2011)
From: Lorenzo Caminiti (lorcaminiti_at_[hidden])
Date: 2011-11-14 19:00:59

On Mon, Nov 14, 2011 at 12:49 PM, Vicente J. Botet Escriba
<vicente.botet_at_[hidden]> wrote:
> Le 10/11/11 06:31, Jeffrey Lee Hellrung, Jr. a écrit :
>
> Hi Lorenzo, here is my review.

Hi Vicente, thanks for your review!

> I have not too much to say. The library is well designed and documented.

:)

> Just some minor points:
>
> To be coherent with the other macros I would suggest
> BOOST_LOCAL_FUNCTION/BOOST_LOCAL_FUNCTION_END instead of
> BOOST_LOCAL_FUNCTION_PARAMS/BOOST_LOCAL_FUNCTION_NAME.

I have been considering this too because sometimes I get confused and
write BOOST_LOCAL_FUNCTION without the _PARAMS. The
BOOST_LOCAL_FUNCTION and BOOST_LOCAL_FUNTION_END names are also more
consistent with the early experimentation on local functions done by
Alexander Nasonov and Steven Watanabe
http://thread.gmane.org/gmane.comp.lib.boost.devel/168612. OTOH, I do
think that _PARAMS and _NAME are more descriptive names because they
clearly say what you specify using these macros and that is why I
ultimately decided to go with the more descriptive names.

I have not strong feelings between these different names and I will
haply comply with the consensus reached during this review. Does
anyone else have an opinion?

> Is there any possible optimization/portability improvement to add the return
> type to the macro for the local function?

The _only_ benefit in passing the result type within the macros is
that when you also specify the types of all bound variables the
library will not use Boost.Typeof. In other words, right now even if
you specify the types of all bound variables `[const] bind(type)[&]
var`, the library still has to use typeof to deduce the result type
because such a type is not directly passed to the macros. I consider
this a _very_ minor advantage largely outweighed by the better syntax
achieved by moving the result type outside the macro.

This is somewhat stated in the docs by the 3rd sentence of this subsection:
https://svn.boost.org/svn/boost/sandbox/local/libs/local/doc/html/boost_local/Advanced_Topics.html#boost_local.Advanced_Topics.specifying_bound_types

> I would prefer BOOST_LOCAL_ON_SCOPE_EXIT to BOOST_LOCAL_EXIT even if longer
> it states clearly the intent of the block. I will not have any problem with
> the shorter BOOST_LOCAL_ON_SCOPE_EXIT.

I don't like BOOST_LOCAL_ON_SCOPE_EXIT because it's too long. How
about BOOST_SCOPED_EXIT, BOOST_SCOPED_BLOCK, BOOST_SCOPED_FUNCTION,
etc? The library will then be named Boost.Scoped and merged with
ScopeExit... if the merge is agreed on during this review.

> IIUC the local blocks intent is to protect the block from unexpected
> updates. Couldn't in this case all the parameters be bound as const
> references by default?
>
> BOOST_LOCAL_BLOCK(sum)
> versus
> BOOST_LOCAL_BLOCK(const bind& sum)
>
> What about adding CONST in the name to reinforce the intent ?
> BOOST_LOCAL_CONST_BLOCK(sum)
>
> I will not have any problem with the shorter BOOST_CONST_BLOCK.

Not quite because you might want to modify one variable but make sure
that another one remains const. Therefore, I think it is best to leave
full control to the user that can specify which variable to be bound
by const and which not. For example:

int x = 10;
int y = -10;
BOOST_LOCAL_BLOCK(const bind& x, bind& y) { // semantics: this block
of code does not change x but it changes y
    assert(x >= 0);
    y = x + 10;
} BOOST_LOCAL_BLOCK_END

Also I think it's easier for the user if LOCAL_BLOCK has the same
binding interface of LOCAL_EXIT and LOCAL_FUNCTION_PARAMS (so no
implied const, etc).

> As variadic macros are part of the new C++11 standard I will present the
> sequence interface as a workaround.

That could be done... I do think that Boost.Local is most useful with
C++03 because with C++11 you can use lambdas. However, even with C++03
most (all?) compilers support variadics so the sequencing syntax could
indeed be presented as a "workaround" to apply when you want to be
100% compliant with C++03 pp.

That said, do you think it is heavy that the docs present all examples
in both syntaxes? If not, then I'd leave it as is.

> I have no preference between this_ and _this. Maybe it would be great to
> offer the possibility to the developer to name it.

You already can by #defining the configuration macro
BOOST_LOCAL_CONFIG_THIS_PARAM_NAME:
https://svn.boost.org/svn/boost/sandbox/local/libs/local/doc/html/BOOST_LOCAL_CONFIG_THIS_PARAM_NAME.html

As it was suggested, I'm thinking that I will change from this_ to
_this (to be consistent with Phoenix) and require to use _this in both
the local function declaration and implementation (and error at
compile-time if you use this instead of _this in the declaration but I
still can't error if you mistakenly use this instead of _this in the
definition :( ). I will also leave the config macro so the users can
change this name if they want/need to.

> Can local functions have types that are typenames of the nesting function?
> e.g. is the following valid?
>
> template <typename T>
> void f() {
>
>  BOOST_LOCAL_FUNCTION_PARAMS(T v)
>  ...
>  BOOST_LOCAL_FUNCTION_NAME(g);
>
>  ...
> }

Yes, this is possible but you need to use the version of the macros
with the _TPL postfix:
https://svn.boost.org/svn/boost/sandbox/local/libs/local/doc/html/boost_local/Tutorial.html#boost_local.Tutorial.templates

> The overload examples, let me think that you will be able to define a
> templated local function. I don't know which syntax could be more
> appropriated, but I hope you will catch the idea
>
> void f() {
>
>  BOOST_LOCAL_FUNCTION(template (typename T), T v)
>    std::cout << hex << v;
>  BOOST_LOCAL_FUNCTION_END(print, std::string, double);
>
>  // use of print with doublke or string
>
>  ...
> }
>
> I don't have a concrete use case now,  but could something like this be
> implemented?

No :( because local classes and functions cannot have their own
template parameters in C++ :( (not even in C++11) they can only use
template parameters they see from the enclosing scope. (BTW, the
discussion on the limitation that local functions cannot be templates
incidentally led to introduce overload<> but overloading local
function and local function templates remain different things.)

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

:)

>> Other questions you may want to consider:
>>
>> - What is your evaluation of the design?
>
> The interface is clean and manage with all the corner cases in an elegant
> way.
>>
>> - What is your evaluation of the implementation?
>
> I have not inspected it yet. I hope to have some time from here to the end
> of the review.

While looking at the implementation, also look at:
https://svn.boost.org/svn/boost/sandbox/local/libs/local/doc/html/boost_local/Implementation.html

>> - What is your evaluation of the documentation?
>
> Very good.
>
> I will add a reference to the ScopedExit library when introducing Local
> exits and the advantages of your approach respect the one in ScopedExit.
>
> When you say " No semicolon |;| is required after the ending macro." do you
> mean that if a ';' is used, no error occurs, or that ';' is not allowed?
> Other usages of "No xxx is required" should be checked also.

It just means that ; is not required. You can still put ; there, it
will work (I just triple checked :) ), but it is not required. That's
true all over (otherwise I say "it's required not to put it there").

> Could you add somewhere in the Implementation section, why the preprocessor
> can not parse default parameters as int y default 2 so you need a ','
> between the variable and default?

There is already a footnote explaining the rationale of why = cannot
be used for default parameter values:
https://svn.boost.org/svn/boost/sandbox/local/libs/local/doc/html/boost_local/Tutorial.html#ftn.boost_local.Tutorial.default_parameters.f0

I can expand this text indicating why default requires a preceding
comma. The comma is needed because the tokens `int x` can be anything
`mytype myvar` so the pp has no way to remove them to separate the
default value from the rest of the parameter declaration. The default
values need to be separated (stripped) because they are not part of
the function type and the library needs to build the function type for
some internal template metaprogramming manipulations. Also I need to
count the number of default parameter to select which functor to use
to declare the local function so it will have the call operator() for
each default parameter (the count is done by the pp and it requires to
identify which parameter has a default so it needs to be separated by
the rest of the parameter declaration-- the count cannot be done using
MPL because again default parameter value are not part of the function
type so I can't use template metaprogramming to inspect them, I need
to use pp metaprogramming). I know, this is quite a long story...

>> - What is your evaluation of the potential usefulness of the library?
>
> Quite useful.
>>
>> - Did you try to use the library?  With what compiler?  Did you have any
>> problems?
>
> I have compiled the examples with darwin 4.2.1, clang-2.9,3.0 with and
> without c++0x enabled.

Cool. I've only tested with:

GNU Compiler Collection (GCC) 4.5.1 (with and without C++11 features
enabled) on Ubuntu Linux 10.
GCC 4.3.4 on Cygwin.
Miscrosoft Visual Studio Compiler (MSVC) 8.0 on Windows XP.

If the library is accepted a larger set of compilers will be supported
(if a compiler can handle Boost.Typeof and Boost.ScopeExit, there's no
reason that I can see why it should not be able to handle
Boost.Local).

> I don't know why the Jamfile just build the executables and doesn't run
> them. Could you update the Jamfile so that we can run the examples?

Yes. I will also have to add tests to the library for now I only have examples.

> Some errors

I looked at the errors below, they are not in Boost.Local:
1) One error is because Boost.Lambda generates an internal error in
Darwing (add_using_boost_lambda.cpp has no Boost.Local code). I don't
know why...
2) All other errors are because the add_optimizers.cpp intentionally
uses the auto classifier to show that Boost.Local works with auto and
register parameters. However, auto is deprecated so you get all the
other errors below.

> darwin.compile.c++ bin/darwin-4.2.1/debug/add_using_boost_lambda.o
> add_using_boost_lambda.cpp: In function 'int main()':
> add_using_boost_lambda.cpp:14: internal compiler error: in

Actually, this example only uses Boost.Lambda... no Boost.Local.

> reference_to_unused, at dwarf2out.c:10603
> Please submit a full bug report,
> with preprocessed source if appropriate.
> See <URL:http://developer.apple.com/bugreporter> for instructions.
>
>    "g++"  -ftemplate-depth-128 -O0 -fno-inline -Wall -g -dynamic
> -no-cpp-precomp -gdwarf-2 -fexceptions -fPIC    -I"../../.."
> -I"/Users/viboes/boost/trunk" -c -o
> "bin/darwin-4.2.1/debug/add_using_boost_lambda.o"
> "add_using_boost_lambda.cpp"
>
> ...failed darwin.compile.c++
> bin/darwin-4.2.1/debug/add_using_boost_lambda.o...
> ...skipped <pbin/darwin-4.2.1/debug>add_using_boost_lambda for lack of
> <pbin/darwin-4.2.1/debug>add_using_boost_lambda.o...
> ...failed updating 1 target...
> ...skipped 1 target...
>
> clang-darwin.compile.c++ bin/clang-darwin-3.0x/debug/add_optimizers.o
> add_optimizers.cpp:12:39: warning: 'auto' storage class specifier is not
> permitted in C++11, and will not be supported in future releases
>    int BOOST_LOCAL_FUNCTION_PARAMS( (auto int x) (register int y) ) {

This is because this example uses the auto classifier which is
deprecated in C++ so the compiler signals that. The purpose of this
example is to the show that Boost.Local supports auto and register
classifiers even if they are deprecated-- so just don't compile this
example with compilers that error on auto/register. I'll put some #if
guard in the example.

> ../../../boost/local/function.hpp:167:65: note: expanded from:
>    BOOST_LOCAL_FUNCTION_PARAMS_(__LINE__, 0 /* no template */, __VA_ARGS__)
>                                                                ^
> ../../../boost/local/function.hpp:163:41: note: expanded from:
>            (void) /* for empty seq */, __VA_ARGS__), \
>                                        ^
> ../../../boost/local/aux_/preprocessor/variadic/to_seq.hpp:39:7: note:
> expanded from:
>    )(__VA_ARGS__)
>      ^
> note: (skipping 97 expansions in backtrace; use -fmacro-backtrace-limit=0 to
> see all)
> /Users/viboes/boost/trunk/boost/preprocessor/control/iif.hpp:18:60: note:
> expanded from:
> #    define BOOST_PP_IIF(bit, t, f) BOOST_PP_IIF_I(bit, t, f)
>                                                           ^
> /Users/viboes/boost/trunk/boost/preprocessor/control/iif.hpp:25:63: note:
> expanded from:
> #    define BOOST_PP_IIF_I(bit, t, f) BOOST_PP_IIF_ ## bit(t, f)
>                                                              ^
> /Users/viboes/boost/trunk/boost/preprocessor/control/iif.hpp:31:31: note:
> expanded from:
> # define BOOST_PP_IIF_0(t, f) f
>                              ^
> add_optimizers.cpp:12:39: error: invalid storage class specifier in function
> declarator
> add_optimizers.cpp:12:39: warning: 'auto' storage class specifier is not
> permitted in C++11, and will not be supported in future releases
>    int BOOST_LOCAL_FUNCTION_PARAMS( (auto int x) (register int y) ) {
>                                      ^
> ../../../boost/local/function.hpp:167:65: note: expanded from:
>    BOOST_LOCAL_FUNCTION_PARAMS_(__LINE__, 0 /* no template */, __VA_ARGS__)
>                                                                ^
> ../../../boost/local/function.hpp:163:41: note: expanded from:
>            (void) /* for empty seq */, __VA_ARGS__), \
>                                        ^
> ../../../boost/local/aux_/preprocessor/variadic/to_seq.hpp:39:7: note:
> expanded from:
>    )(__VA_ARGS__)
>      ^
> note: (skipping 97 expansions in backtrace; use -fmacro-backtrace-limit=0 to
> see all)
> /Users/viboes/boost/trunk/boost/preprocessor/control/iif.hpp:18:60: note:
> expanded from:
> #    define BOOST_PP_IIF(bit, t, f) BOOST_PP_IIF_I(bit, t, f)
>                                                           ^
> /Users/viboes/boost/trunk/boost/preprocessor/control/iif.hpp:25:63: note:
> expanded from:
> #    define BOOST_PP_IIF_I(bit, t, f) BOOST_PP_IIF_ ## bit(t, f)
>                                                              ^
> /Users/viboes/boost/trunk/boost/preprocessor/control/iif.hpp:31:31: note:
> expanded from:
> # define BOOST_PP_IIF_0(t, f) f
>                              ^
> add_optimizers.cpp:12:39: error: invalid storage class specifier in function
> declarator
> 2 warnings and 2 errors generated.

All above is because of the deprecated auto classifier.

>    "/Users/viboes/clang/llvmCore-3.0-rc1.install/bin/clang++" -x c++
> -std=c++0x -O0 -g -O0 -fno-inline -Wall -g  -I"../../.."
> -I"/Users/viboes/boost/trunk" -c -o
> "bin/clang-darwin-3.0x/debug/add_optimizers.o" "add_optimizers.cpp"
>
> ...failed clang-darwin.compile.c++
> bin/clang-darwin-3.0x/debug/add_optimizers.o...
> ...skipped <pbin/clang-darwin-3.0x/debug>add_optimizers for lack of
> <pbin/clang-darwin-3.0x/debug>add_optimizers.o...
> clang-darwin.compile.c++ bin/clang-darwin-3.0x/debug/add_optimizers_va.o
> add_optimizers_va.cpp:21:37: warning: 'auto' storage class specifier is not
> permitted in C++11, and will not be supported in future releases
>    int BOOST_LOCAL_FUNCTION_PARAMS(auto int x, register int y) {
>                                    ^
> ../../../boost/local/function.hpp:167:65: note: expanded from:
>    BOOST_LOCAL_FUNCTION_PARAMS_(__LINE__, 0 /* no template */, __VA_ARGS__)
>                                                                ^
> ../../../boost/local/function.hpp:163:41: note: expanded from:
>            (void) /* for empty seq */, __VA_ARGS__), \
>                                        ^
> ../../../boost/local/aux_/preprocessor/variadic/to_seq.hpp:39:7: note:
> expanded from:
>    )(__VA_ARGS__)
>      ^
> note: (skipping 103 expansions in backtrace; use -fmacro-backtrace-limit=0
> to see all)
> /Users/viboes/boost/trunk/boost/preprocessor/control/iif.hpp:18:60: note:
> expanded from:
> #    define BOOST_PP_IIF(bit, t, f) BOOST_PP_IIF_I(bit, t, f)
>                                                           ^
> /Users/viboes/boost/trunk/boost/preprocessor/control/iif.hpp:25:63: note:
> expanded from:
> #    define BOOST_PP_IIF_I(bit, t, f) BOOST_PP_IIF_ ## bit(t, f)
>                                                              ^
> /Users/viboes/boost/trunk/boost/preprocessor/control/iif.hpp:31:31: note:
> expanded from:
> # define BOOST_PP_IIF_0(t, f) f
>                              ^
> add_optimizers_va.cpp:21:37: error: invalid storage class specifier in
> function declarator
> add_optimizers_va.cpp:21:37: warning: 'auto' storage class specifier is not
> permitted in C++11, and will not be supported in future releases
>    int BOOST_LOCAL_FUNCTION_PARAMS(auto int x, register int y) {
>                                    ^
> ../../../boost/local/function.hpp:167:65: note: expanded from:
>    BOOST_LOCAL_FUNCTION_PARAMS_(__LINE__, 0 /* no template */, __VA_ARGS__)
>                                                                ^
> ../../../boost/local/function.hpp:163:41: note: expanded from:
>            (void) /* for empty seq */, __VA_ARGS__), \
>                                        ^
> ../../../boost/local/aux_/preprocessor/variadic/to_seq.hpp:39:7: note:
> expanded from:
>    )(__VA_ARGS__)
>      ^
> note: (skipping 103 expansions in backtrace; use -fmacro-backtrace-limit=0
> to see all)
> /Users/viboes/boost/trunk/boost/preprocessor/control/iif.hpp:18:60: note:
> expanded from:
> #    define BOOST_PP_IIF(bit, t, f) BOOST_PP_IIF_I(bit, t, f)
>                                                           ^
> /Users/viboes/boost/trunk/boost/preprocessor/control/iif.hpp:25:63: note:
> expanded from:
> #    define BOOST_PP_IIF_I(bit, t, f) BOOST_PP_IIF_ ## bit(t, f)
>                                                              ^
> /Users/viboes/boost/trunk/boost/preprocessor/control/iif.hpp:31:31: note:
> expanded from:
> # define BOOST_PP_IIF_0(t, f) f
>                              ^
> add_optimizers_va.cpp:21:37: error: invalid storage class specifier in
> function declarator
> 2 warnings and 2 errors generated.
>
>    "/Users/viboes/clang/llvmCore-3.0-rc1.install/bin/clang++" -x c++
> -std=c++0x -O0 -g -O0 -fno-inline -Wall -g  -I"../../.."
> -I"/Users/viboes/boost/trunk" -c -o
> "bin/clang-darwin-3.0x/debug/add_optimizers_va.o" "add_optimizers_va.cpp"

Same story, the deprecated auto classifier.

>> - How much effort did you put into your evaluation?  A glance?  A quick
>> reading?  In-depth study?
>
> A quick reading of the docs and build the examples.
>>
>> - Are you knowledgeable about the problem domain?
>
> Yes, the domain (nested functions and scoped guards) is quite simple and
> well know. I have followed the evolution of the library since the beginning.
>>
>> 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?
>
> I think that both libraries could be merged in a Scoped library as suggested
> by Lorenzo in the documentation. Once the Scoped library is released, the
> ScopedExit library could be set as deprecated during a certain time, before
> removing it definitively, as it occurred with Boost.Compose.
>>
>> - 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
>
> An alternative could be to include it in the Functional library, together
> with forward and factory.
>>
>> - 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
>> - 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
>
> I guess that both macros could be better be included in Boost.Preprocessor.

IMO, it shouldn't go in Boost.Preprocessor because it is about C++
types and not the pp (which is actually C). BTW, these macros are
really trivial, they don't use variadics at all, and I find
IDENTITY_TYPE very handy:

#define BOOST_IDENTITY_TYPE(parenthesized_type) \
    boost::function_traits< void parenthesized_type >::arg1_type

namespace boost { namespace aux {
    template<typename T>
    inline typename boost::add_reference<T>::type identity_value(
            typename boost::add_reference<T>::type value) {
        return value;
    }
}} // namespace boost::aux

#define BOOST_IDENTITY_VALUE(parenthesized_value) \
    boost::aux::identity_value parenthesized_value

BOOST_IDENTITY_TYPE((boost::mpl::vector<char, int>))
BOOST_IDENTITY_VALUE((boost::mpl::vector<char, int>::value))

Thanks a lot for your review!
--Lorenzo