Le 10/11/11 06:31, Jeffrey Lee Hellrung, Jr. a écrit : Hi Lorenzo, here is my 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. Is there any possible optimization/portability improvement to add the return type to the macro for the local function? 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. 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. As variadic macros are part of the new C++11 standard I will present the sequence interface as a workaround. I have no preference between this_ and _this. Maybe it would be great to offer the possibility to the developer to name it. 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); ... } 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?
--------
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. - 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. 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?
- 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.
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? Some errors 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 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) ) { ^ ../../../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. "/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"
- 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. Best, Vicente