Boost.Local Review (Nov 10, 2011 to Nov 19, 2011)

10 Nov 2011

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

Jeffrey Lee Hellrung, Jr.

Vicente J. Botet Escriba

Jeffrey Lee Hellrung, Jr.

Vicente J. Botet Escriba

Jeffrey Lee Hellrung, Jr.

Christian Holmquist

Jeffrey Lee Hellrung, Jr.

Christian Holmquist

Lorenzo Caminiti

Lorenzo Caminiti

Lorenzo Caminiti

Krzysztof Czainski

Thomas Klimpel

Lorenzo Caminiti

Jeffrey Lee Hellrung, Jr.

Lorenzo Caminiti

Steven Watanabe

Lorenzo Caminiti

Vicente J. Botet Escriba

Lorenzo Caminiti

Lorenzo Caminiti

Vicente J. Botet Escriba

tags

participants (7)