Re: [boost] [local] Review

Subject: Re: [boost] [local] Review
From: Lorenzo Caminiti (lorcaminiti_at_[hidden])
Date: 2011-11-18 20:59:26

On Fri, Nov 18, 2011 at 6:53 PM, John Bytheway
<jbytheway+boost_at_[hidden]> wrote:

Hello John and thank you very much for your review.

> To begin with, my overall opinion:  I say yes, Boost.Local should be
> accepted.

:)

>> - What is your evaluation of the design?
>
> The design is clearly well thought-out.  I admire Lorenzo's dedication
> to discussing his design decisions as thoroughly as possible on the
> mailing list.  I've watched the development of the library with
> interest, and have been glad of the opportunity to comment along the way.
>
> On one specific pleasant surprise: thank you for providing the inline
> feature to offer your users the chance to choose their optimization
> trade-offs for themselves, and I'm glad you found a faster alternative
> to the virtual function trick.
>
> However, I still have a few concerns:
>
> I question the choice to bind const variables by const value by default,
> with (as far as I can see) no means to bind them by non-const value.
> This runs contrary to my instincts in regard to functions in C++ more
> generally (although I see C++11 lambdas have similar behaviour...).
> It's not a serious problem, but I would have chosen differently.

As you pointed out this is consistent with C++11 lambdas and the
principle is that I never remove const (but only add it in case of
const bind). I will mention that this consistent with C++11 lambdas in
the docs (it's not currently there).

> On binding this: I vote that this_ is a better name than _this.  The
> name is not a placeholder in the sense the term is used in libraries
> using the leading underscore convention for placeholders.

Agreed (as originally suggested by Steven).

> Also, how do your various choices of syntax behave in the context of
> nested local functions?  Consider:
>
> void BOOST_LOCAL_FUNCTION_PARAMS(bind this) {
>  void BOOST_LOCAL_FUNCTION_PARAMS(bind this_) {
>    this_->f();
>  } BOOST_LOCAL_FUNCTION_NAME(g)
> } BOOST_LOCAL_FUNCTION_NAME(h)
>
> If you change the syntax (as I think you were proposing) so that "this_"
> appears in the first line here in place of "this", then how would the
> second line behave?  I would want it to bind the this pointer for the
> outer object, not the inner instance of the class that Boost.Local
> defines for me.

That's a good point! Yes, this will bind the outer object (never the
functor). One more reason to change the name to this_ within the
bindings (as originally suggested by Krzysztof) so it will be more
clear. I will also add a note about this nested binding of this_ in
the docs.

> You discuss exception specifications for the various objects that can be
> defined.  If this includes noexcept in C++11 (which I imagine it does)
> then you should mention that.  The ability to write noexcept local

Yes, on C++11 compilers you can also use noexcept (I have to test that
and then I will add it to the docs).

> blocks (which can easily be made to work in C++03 too) might be another
> motivating feature for local blocks.  Furthermore, you should seriously

OK, I will mention that in the docs for local blocks.

> consider whether BOOST_LOCAL_EXIT's function should be declared noexcept
> by default where possible, since it is in many ways like a destructor.

Yes, I will consider this (as originally pointed out by Andrzej) and
change the implementation and docs as necessary (this will at least be
a warning in the docs if not a noexcept in the implementation).

> How feasible would it be to allow overload to be used without declaring
> all the function types.  So, your example:
>
> boost::local::function::overload<
>          void (const std::string&)
>        , void (const double&) // Overload 1st param type.
>        , void (const double&, const char*) // Overload giving default
> param.
>        , int (int, int) // Overload giving 2 params (from function
> pointer).
>    > print(print_string, print_double, print_double, print_add);
>
> might be written instead:
>
> auto print = make_overload(print_string, print_double, print_double,
> print_add);

Yes, this should be possible. I will experiment with it and eventually
provide make_overload for convenience.

> (with suitable use of BOOST_AUTO for C++03).  I guess this only works if
> all the functors passed are monomorphic and expose (or could be made to
> expose) their argument types, but that covers function pointers,
> boost::function objects, and Boost.Local functions, which is a fairly
> wide applicability.
>
> Writing the above inspires me to ask: Do Boost.Local functions expose
> their argument types and return types at compile time?  If there is a
> widely accepted way for monomorphic functors to do so, then they should.
>  If there is no such widely accepted way, then one should be created,
> but that's probably not a job for you right now :).

What do you mean? How would this feature look like for Boost.Local?
(Sorry if I am not following...)

> In a similar vein, as I was pondering potential uses of the overload
> feature, I wondered whether it could be used to write a visitor for
> Boost.Variant.  I suspect that it could not, because such a visitor must
> declare its return type in a manner tat the overload object will not.
> It would be nice if that could be made to work.  Can you think of a good
> way?

I can't... but some other Boosters might. (I will think about it more.)

>> - What is your evaluation of the implementation?
>
> I have not looked at the implementation, but I have followed the
> discussions and examined some earlier versions of it.  Any problems I
> noted before have, I think, as far as reasonable, been resolved.
>
> However, I would like to ask: on the overloading features:  It looks
> like boost::local::function::overload could be written with variadic
> templates where available and thereby avoid the maximum arities in the
> configuration macros (and be much simpler and faster to compile).  Have
> you done so?

Not yet but I will implement it that way for C++11 compilers.

>> - What is your evaluation of the documentation?
>
> The documentation is very good.  Allow me to praise in particular the
> detailed rationales.  A few niggles:
>
> * General
>
> You have various examples which write to stdout.  I realise it's fairly
> clear, but it might be nice to drive the point home by explicitly
> showing what the expected output of these programs is.  Alternatively,
> use asserts instead of writing to stdout.

OK, I will keep this comment in mind (especially when reworking some
of the examples into tests and/or running them from within the Jamfile
as suggested by Vicente).

> * Introduction
>
> "the object this can be bound (eventually by constant value" - I don't
> understand "eventually".

It means that you can either "bind this" or "const bind this" (the
second binds the object as constant within the local functions even if
the object is non-const in the enclosing member function). I will make
this text more clear.

> * Tutorial
>
> The code "if (size && nums) delete[] nums;" perpetuates the myth that
> one should not delete NULL pointers.  Please don't do that.  Go ahead
> and delete[] nums even if it's NULL.

OK.

> "The maximum number of parameters that can be passed to a local function
> (excluding eventual bound variables)" - I don't understand "eventual".

It means that the max number of parameters does not include the
bindings. In other words, if the max number of parameters is 5 then
you can write local functions with up to 5 parameters and as many
bindings as you want PARAMS(int a1, int a2, int a3, int a4, int a5,
bind x1, bind x2, ..., bind x100). It says "eventual" because there
might or not be binds. I will make this text more clear.

> You have the "Important" warning about registering types with
> Boost.Typeof.  I presume this applies only to compilers without typeof
> support.  If so you should probably make this a little more clear.

Yes, exact same as for Boost.Typeof. I will make this text more clear.

> * Advanced Topics
>
> Does the existence of the recursive "keyword" mean that a local function
> cannot be named recursive?  If so, you should mention that in passing.

Yes, I will add this to the docs (as well as that local function
parameter types cannot be named bind as pointed out by Andrzej).

> "Compilers have not been observed to be able to inline recursive local
> function calls (not even when the recursive local function is also
> declared inlined)." - do you mean "not only when"?

I mean NAME(inline recursive f) still wasn't observed to inline.

> "C++03 local classes cannot be templates." - this is also true in C++11,
> right?

Yes, also for C++11 so I will clarify that in the docs.

> * Macro BOOST_LOCAL_FUNCTION_NAME
>
> "The local function name can be prefixed by the "keyword" inline to
> declare the local function inlined or by the "keyword" recursive to
> declare the local function recursive" - can both "keywords" be used?

Yes but again no compiler was observed to inline in this case.

> This wording suggests to me that they cannot.  If they can, then I
> suggest changing "or" to "and/or".  If not, this should be made clearer.

OK.

> * Macro BOOST_IDENTITY_TYPE
>
> I don't understand the "Warning" at the bottom; can you expand?

This works:

template<typename K, typename V>
void f ( std::map<K, V> m ) {}

int main() {
    std::map<char*, int> m;
    f(m);
    return 0;
}

But this doesn't:

template<typename K, typename V>
void f ( typename BOOST_IDENTITY_TYPE((std::map<K, V>)) m ) {}

int main() {
    std::map<char*, int> m;
    f(m);
    return 0;
}

In this case you have to explicitly write f<char*, int>(m); in main :(
. Note that this is never a problem for local functions because
(unfortunately) they cannot be templates. However, given that
IDENTITY_TYPE is shown here as a general purpose macro this is a
limitation of the macro for the general case.

I will add this example to the docs to clarify.

> * Macro BOOST_IDENTITY_VALUE
>
> The implementation provided on this page doesn't look like it could
> possibly work.  Surely the type T cannot be deduced in a call to
> identity_value without template arguments?

Yes, it should just be T const& identity_value(T const& value).

> * Examples
>
> Should a chunk of the variadic macro version of the Emulating D's Scope
> Guards" example be commented out?  Surely not...

No, they shouldn't. I'll fix that.

> * Appendix: Alternatives
>
> In the benchmarking note, you say "For all tested compilers, this
> function pointer prevents the compiler optimization algorithms from
> inlining the local function calls.".  You might want to mention that it
> is in theory possible for the compiler to inline it, and some other
> compilers or future versions may do so.  (Or you may not, if you think
> it sounds too weasely or hypothetical)

Yes, that's what I mean... I'll think about possible rewording.

> I don't see any of the benchmarking images.  Should I?  I'm experiencing
> some very odd symptoms from my graphics card since I first tried to view
> them, so it may well be a problem at my end.

I can see the images... does anyone else have this problem?
https://svn.boost.org/svn/boost/sandbox/local/libs/local/doc/html/boost_local/Alternatives.html#boost_local.Alternatives.local_function_benchmarking

> * Release Notes
>
> Why is there a "TODO" at the bottom?

Oops, I'll remove it.

> * Acknowledgments
>
> The title is spelt wrong (should be Acknowledgements)

OK.

> Thanks for the thanks :).
>
>> - What is your evaluation of the potential usefulness of the
>> library?
>
> Personally, my C++ development has mostly moved to C++11, and I do not
> expect to have reason to want this functionality, settling for lambdas
> instead.  However, I might yet be in a situation where I am forced to
> use C++03, and I'm sure many others will be in just that situation for a
> while yet.  In that context, I think this library has significant
> potential usefulness.  Certainly there have been times when I have
> wanted something like it.  Much as I admire Phoenix, I think there will
> always be circumstances where Boost.Local will be an easier, more useful
> and more readable alternative.
>
> As part of your struggle to motivate the utility of const binding in
> local blocks, you might want to mention in the documentation the
> possibility of using it to assure the behaviour of code received in
> macro parameters as (I imagine) you are doing in Boost.Contract.

I'll think about it.

>> - Did you try to use the library?  With what compiler?  Did you have
>> any problems?
>
> I did not try to use this version, but I have experimented briefly with
> previous versions, with gcc, clang, and icc (IIRC).  I had no problems
> serious enough that I recall them.
>
>> - How much effort did you put into your evaluation?  A glance?  A
>> quick reading?  In-depth study?
>
> I read the documentation with moderate care.  Also, I have been
> following the development on the list over recent months.
>
>> - Are you knowledgeable about the problem domain?
>
> I have faced problems that this library would have solved neatly, and I
> discussed some implementation details with Lorenzo at times.
>
>> 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?
>
> Since I think the Boost.Local version is strictly superior to the
> Boost.ScopeExit version, I would suggest that the ScopeExit version be
> deprecated (but not removed, unless it is proving a significant
> maintenance burden).  On the other hand, ScopeExit is a good name for
> that functionality, and so it is well worth seriously considering
> upgrading that library and not including the functionality in this new
> place.  Should that be possible to achieve smoothly, it might be a
> better choice.
>
>> - Lorenzo is proposing to add boost::local::function::overload to
>> Boost.Function as boost::function::overload.
>
> Indeed it sounds reasonable to put it there.  If so, it should be
> implemented with due regard for the possibilities of C++11 features
> (variadic macros, perfect forwarding) as mentioned above.
>
>> - Lorenzo is proposing to add BOOST_IDENTITY_TYPE to boost/utility.
>
> This seems reasonable.
>
>> - Likewise, Lorenzo is proposing to add BOOST_IDENTITY_VALUE to
>> boost/utility.
>
> I am struggling to see any situation where BOOST_IDENTITY_VALUE is
> necessary.  Can you provide one?  When will simply wrapping the
> expression in parentheses not suffice?  Is it because you might be in a
> preprocessing context that requires an alphanumeric initial character?

Extra parenthesis should always be enough (as far as I can tell). In
cases where you need to start with an alphanumeric token for PP_CAT,
you can wrap the expression within its type (assuming it has a copy
constructor):

mpl::size< mpl::vector<float, long> >::value // unwrapped commas :(
(mpl::size< mpl::vector<float, long> >::value) // wrapped :)
int(mpl::size< mpl::vector<float, long> >::value) // wrapped and start
with alphanumeric :))

So there might be little to no use for IDENTITY_VALUE. Honestly, I've
implemented IDENTITY_VALUE just for symmetry with IDENTITY_TYPE but
while I use IDENTITY_TYPE quite often I've never had to use
IDENTITY_VALUE. IMO, it's reasonable to add IDENTITY_TYPE but not
IDENTITY_VALUE (but I'm not the one casting a vote here).

> That's all for my review.  Thanks to Lorenzo, Jeffrey, Gregory, and good
> luck!

Thanks a lot for your review!
--Lorenzo