On Mon, Nov 14, 2011 at 12:49 PM, Vicente J. Botet Escriba <vicente.botet@wanadoo.fr> 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_loca...
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_LOCA... 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_loca...
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_loca...
- 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_loca... 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/loca...
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_IDEN... - 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_IDEN...
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