|
|
Boost : |
Subject: Re: [boost] Boost.Local Review (Nov 10, 2011 to Nov 19, 2011)
From: Vicente J. Botet Escriba (vicente.botet_at_[hidden])
Date: 2011-11-14 12:49:18
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/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.
Best,
Vicente
Boost list run by bdawes at acm.org, gregod at cs.rpi.edu, cpdaniel at pacbell.net, john at johnmaddock.co.uk