[boost] [local] Review

Subject: [boost] [local] Review
From: John Bytheway (jbytheway+boost_at_[hidden])
Date: 2011-11-18 18:53:59

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.

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.

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.

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
blocks (which can easily be made to work in C++03 too) might be another
motivating feature for local blocks.  Furthermore, you should seriously
consider whether BOOST_LOCAL_EXIT's function should be declared noexcept
by default where possible, since it is in many ways like a destructor.

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);

(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 :).

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?

> - 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?

> - 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.

* Introduction

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

* 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.

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

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.

* 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.

"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"?

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

* 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?
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.

* Macro BOOST_IDENTITY_TYPE

I don't understand the "Warning" at the bottom; can you expand?

* 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?

* Examples

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

* 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)

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.

* Release Notes

Why is there a "TODO" at the bottom?

* Acknowledgments

The title is spelt wrong (should be Acknowledgements)

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.

> - 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?

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

John Bytheway