15 Nov
2011
15 Nov
'11
12:53 a.m.
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.
> 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
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.
>
>> 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.
>
>> 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
}
>> 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 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
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_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
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_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))
I agree. See my answer to Jeffrey.
> Thanks a lot for your review!
You are welcome.
Good luck,
Vicente