On Mon, Nov 14, 2011 at 7:53 PM, Vicente J. Botet Escriba <vicente.botet@wanadoo.fr> wrote:
Le 15/11/11 01:00, Lorenzo Caminiti a écrit :
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.
Yes, but the BOOST_LOCAL_BLOCK and BOOST_LOCAL_EXIT should be renamed BOOST_LOCAL_BLOCK_PARAMS, ...
In addition the list contains the parameters as well as the bounds variables.
Good point. If no one objects, I'm happy with: FUNCTION_PARAMS(params_and_bindings) --> FUNCTION(params_and_bindings) FUNCTION_NAME(name) --> FUNCTION_END(name)
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...
Yes, I saw that. Don't you think that don't needing Boost.Typeof is a big advantage? There are some compilers that don't supports yet, Typeof. For them a specific macros would be welcome.
Yes, I can add this feature by providing another macro like: /* no result type here */ FUNCTION_WITH_RESULT[_TPL]( result_type [, params_and_bindings] ) { ... } FUNCTION_END( [inline | recursive] name ) BLOCKS and EXITS always have void result so no need for an extra macro for them (if you specify the type for your bindings for BLOCKS and EXITS then they always use no typeof). That said, I need to closely look at the implementation to guarantee that no typeof is used when the result and binding types are specified in the macros.
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.
Yes, the names could depend on the name of the merged library, but I have no problem in no preficing these macros with BOOST_LIBNAME_ if we find a better name. BOOST_SCOPED_EXIT is to close to the existing macro BOOST_SCOPE_EXIT, which could trouble some of us.
I sent another reply with considerations on possible names.
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).
I don't see the utility for this use case. Anyway homogeneity is always a good thing. I said that because I was thinking on a possible language change to add a const attribute to a block
[[const]]
{ // semantics: this block of code does not change any accessible variable assert(x>= 0); y = x + 10; // ERROR }
Thorsten proposed something like that in N1613 when considering if a library could implement Contract Programming while ensuring assertion constant-correctness (see bottom of page 15): http://www.open-std.org/JTC1/SC22/WG21/docs/papers/2004/n1613.pdf I saw that and asked Boosters if there was a way to program a "constant-block". You and Steven replied saying to look at Alexander's early work on local functions thus I started Boost.Local. So yes, the main use case that I always had in mind for local blocks is constant blocks ;) Language support would be nice but now you can do it with Boost.Local!
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 was thinking to the macro BOOST_LOCAL_CONFIG_COMPLIANT, which could let think the user that using macros not provided by this mode are not "portable". I would say that they are not portable to non-c++11 compliant compilers.
I see. #define BOOST_LOCAL_CONFIG_COMPLIANT does two things: 1) Don't use variadics (and empty macro parameters). 2) Don't pass local types as template parameters (this makes a run-time local function call slower). Maybe I should split this one macro into two macros BOOST_LOCAL_CONFIG_NO_VARIADICS and BOOST_LOCAL_CONFIG_NO_LOCAL_TYPES_AS_TPARAMS. What do you think?
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...
OH, yes of course.
--------
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...
Thanks, I missed it.
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...
No, the rationale you pointed out is enough.
- 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.
Glad to see that your library doesnt have any issue ;-)
- - 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))
I agree. See my answer to Jeffrey.
Thanks a lot for your review!
You are welcome.
Thanks. --Lorenzo