Boost :

Date view	Thread view	Subject view	Author view

Subject: [boost] [review] [Local] Review Result - ACCEPTED
From: Lorenzo Caminiti (lorcaminiti_at_[hidden])
Date: 2011-12-14 09:13:32

Next message: Jeremiah Willcock: "Re: [boost] [graph] LEDA adapter"
Previous message: Martin B.: "Re: [boost] [Serialization] (Commented) assertion when using types in multiple DLLs on Windows (Ticket #3934 and #4394)"

On Mon, Dec 12, 2011 at 3:15 AM, Jeffrey Lee Hellrung, Jr.
<jeffrey.hellrung_at_[hidden]> wrote:
> Local is ACCEPTED into Boost.

> After following the lively Local review discussion several weeks ago, and
> reviewing the discussion a second (and sometimes third) time, I have come to
> the above conclusion. There was quite a bit of passion on both the sides of
> "aisle", and thus, obviously, no decision I make would be well-received by
> all.
>
> Let me start by summarizing the main arguments against including Local in
> Boost:
>
> (a) Local functions are not very useful, at least in the presence of
> existing alternatives (e.g., namespace functions and Boost.Phoenix).
> (b) Local is really a portability library for C++03 presenting an imperfect
> emulation of features readily available in C++11.
> (c) Local's interface is primarily macro-based, making code ugly and
> difficult to read.
>
> In my opinion as the review manager, a sufficient number of individuals in
> the discussion found the library "useful" to address (a) (sometimes with
> additional positive adverbs); indeed, at least a couple individuals have
> shared positive experiences with real-world use. Namespace functions require
> one to move code to somewhere other than where one may prefer to have it,
> and often requires a significant amount of boilerplate when binding local
> variables. Aside from any perceived issues with Boost.Phoenix's syntax and
> compiler error messages, it has been noted that binding member functions
> within Boost.Phoenix can get ugly. As far as (b) is concerned, the community
> seems pretty far from reaching a consensus on whether a library described by
> (b) belongs in Boost. There are certainly libraries currently in Boost that
> could be pegged to satisfy (b) as well, though they all have other
> mitigating features they make their situation different from Local in some
> way. As far as (c) is concerned, several have acknowledged that a
> macro-based interface is necessary to implement local functions in C++03,
> and it seems to have been generally agreed that, given this limitation,
> Lorenzo has done an admireable job making the interface as easy-to-use as
> possible. Some find it ugly, others find it reasonable.
>
> In addition to the counterarguments of (a-c) above, the following facts
> weighed into my final decision:
> * Lorenzo has been engaging and in constant communication with the
> developer's list during the development of Local. This gives me confidence
> that he will continue to actively maintain (and improve?) Local.
> * The documentation is unanimously agreed to be of Boost quality.
> * The transition of some organizations from C++03 to C++11 may take several
> more years, and Boost has a history of supporting "ancient" compilers (for
> better or worse). The point being, a library that eases the transition from
> C++03 to C++11 has some merit based on current precedent.
> * There had been previous work on local functions by Alexander Nasanov and
> Steven Watanabe shared on the developer's list, suggesting a desire for this
> functionality for some time.
> * Local is approximately an extension of Boost.ScopeExit; indeed, it
> basically fulfills the request to Alexander Nasanov from the review result
> [2] of Boost.ScopeExit to create such an extension.
>
> Lastly, my own opinion of "what Boost is" factored in here. I view Boost as
> *partly* a collection of general purpose libraries that can be used in wide
> variety of applications (and thus Boost frequently acts as a staging ground
> for standard adoption). Based on review feedback, I believe Local satisfies
> this criterion; and based on the mailing list discussion, I believe this
> view of Boost is not entirely inconsistent with others on the mailing list.

Thank you for the detailed explanation of the rationale of the decision.

> Ultimately, it wasn't so much a # of "yes" votes versus # of "no" votes as
> it was the above general considerations. Regardless, I think independent of
> how the votes were counted, the "yes" votes outnumbered the "no" votes. This
> required some discretion on my part as not everyone who expressed an opinion
> submitted a formal review, and some participants were only arguing in favor
> of some specific point supporting either acceptance or rejection of Local.

My thanks to you and to the Boost community for clarifying two process
questions I had during the review:
1) How to count votes-- not just yes vs. no but it's up to the review
manager how to count and weight votes.
2) Should active authors' opinion count more-- yes if the review
manager decides so.
These points make sense to me.

> "Yes" reviews (7)
> --------
> Krzysztof Czainski
> Andrzej Krzemienski
> Pierre Morcello
> Nat Lindon
> John Bytheway
> Edward Diener
> Gregory Crosswhite
>
> "No" reviews (3)
> --------
> Vicente J. Botet
> Thomas Heller
> Hartmut Kaiser
>
> Paul A. Bristow and Alexander Nasanov (the author of Boost.ScopeExit) both
> submitted reviews but did not express an opinion (as far as I could tell) on
> whether Local should be included in Boost, though if I had to peg Paul's, it
> would be to reject Local. From what I gathered, Joel de Guzman, Joel Falcou,
> Dean Michael Berris, and Lucanus J. Simonson were opposed to including Local
> in Boost (the aforementioned did not submit a formal review, though a formal
> review might be unnecessary if your vote is "no"). On the other hand, Brian
> Wood, Philippe Vaucher, Mathias Gaunard, Robert Ramey, Nathan Ridge, Brent
> Spillner, Thomas Klimpel, Christopher Jefferson, Daniel James, Rafael
> Fourquet, Matthias Schabel, and Robert Stewart participated in the
> discussion and argued in favor of some point that supported accepting Local
> in Boost. I want to be clear here that certainly not everyone in the
> aforementioned list even implied that they supported acceptance of Local (I
> would guess that only roughly half implied as much), but they indirectly
> helped its case by addressing arguments against inclusion. Overall, that
> indicates to me that more individuals support acceptance of Local into Boost
> than rejection.
>
> [Apologies for any name misspellings and absent accents.]

My personal thanks to /everyone/ that participated to the discussions.

> Regarding Boost.ScopeExit: 4 reviews were in favor of deprecating
> Boost.ScopeExit; 3 reviews felt there was nothing wrong with both
> Boost.ScopeExit and Local coexisting; and 3 reviews mentioned the
> possibility of improving Boost.ScopeExit with the features provided by
> Local's exits. As such I'm inclined to let Alexander (who opposed any kind
> of merging of Local and Boost.ScopeExit) work with Lorenzo on improving
> Boost.ScopeExit as he sees fit, and hopefully both libraries can address
> this apparent duplication of functionality within their respective
> documentation to avoid user confusion.

I will add void and this_ to ScopeExit (keeping it's compatibility
with C++11 lambda style binding) and remove local exits from my
library. (I will add a note to my library docs on how to use local
function to easily implement scope exits with constant binding--
rarely needed.)

> Regarding local::function::overload: Based on the review comments, it makes
> the most sense to add this to Boost.Functional.

I will move overload to Functional.

> Regarding BOOST_IDENTITY_TYPE: This should be added to Boost.Utility. On the
> other hand, there doesn't appear to be a compelling use case for
> BOOST_IDENTITY_VALUE.

I will add IDENTITY_TYPE to utility while IDENTITY_VALUE will not be
included in Boost.

> The following are some suggested *possible* improvements that reviewers
> brought up. This list is by no means exhaustive. Further, I personally don't
> think all of these suggestions are necessarily "good", but I think it's fair
> for Lorenzo (and the community) to consider them nonetheless. Parenthetic
> comments are my own opinions.
>
> * Some aren't convinced of the utility of LOCAL_BLOCK. (It's use cases
> appear fairly narrow so it might be best to simplify the library and remove
> this capability.)

I will remove local blocks (I will document how to trivially implement
them with a void local function with no parameters invoked right after
it is defined).

> * Use "this_" (as opposed to "_this") as an alias for "this" within function
> bodies, and possibly also within bind declarations.
> * Present Boost.PP sequence interface as a workaround for the variadic
> interface? (I don't have a problem with supporting both interfaces at the
> top level.)
> * "Local" and "Locale" look too much alike, suggesting a name change to
> "Scope", "Scoped", "LocalFunction", "Closure" may be a good idea?

Given that local exits and blocks will be removed, I will rename
Boost.Local to Boost.LocalFunction.

> * Allow use without dependence on Boost.TypeOf.
> * Rename prefix and postfix macros from XXX_PARAMS/XXX_NAME to XXX/XXX_END?
> (I don't mind the current macros.)
> * Explicitly separate bound variables from function parameters. (I think
> this suggestion has merit but I don't know what the interface could look
> like.)
> * Remove support for default parameters to simplify interface and
> documentation? (It doesn't seem like default function parameters would be
> very useful.)
> * Use "capture" instead of "bind" for the bind/capture keyword? (I like and
> prefer "bind".)

I will keep bind and add a note to the docs.

Here's a list of all the changes I will make as they were identified
during the review:

http://sourceforge.net/apps/trac/contractpp/report/1

#2 Remove local blocks
#3 Remove local exits
#5 Rename the library Boost Local Function
#4 Rename FUNCTION_PARAMS/_NAME to FUNCTION/_END
#21 Use this_ also in local function declaration
#7 Leave BOOST_IDENITY_TYPE but not _VALUE
#8 Move PP_KEYWORD macros to implementation details
#10 Move local::function::overload to Boost Functional
#16 Make sure that the local function closures work like C++11
lambda closures
#6 Expose result_type, argN_type, and arity
#9 Use Boost Preprocessor built-in variadic support
#12 Present the sequencing syntax as a work-around
#14 Allow to specify result type within the LOCAL_FUNCTION macro
#15 Try to reduce dependency on Boost Typeof
#17 Improve motivation section
#24 Document that local function can access protected/private members
#11 Create regression tests
#20 Fix documentation typos
#22 Investigate MSVC errors with /ZI
#26 Consider using assert instead of example writing to cout
#28 Improve docs on need for Typeof registration
#29 Document that a local function cannot be named recursive
#1 Change the order of the Tutorial subsections
#13 Add example for the GCC_LAMBDA macros
#19 Document that bind cannot be use as a parameter type
#23 Add the C++11 lambda const-binding workaround to the Alternatives
#25 Indicate Phoenix and Lambda version numbers in Alternatives
#27 Free arrays in examples without checking !=0

Please let me know if I missed anything.

Two personal reflections from this experience:
1) I should have studied Phoenix more before the review started. I
would have been better prepared for some of the discussions.
2) A while back, someone on this ML suggested that an library author
should participate to 3 reviews before being able to have his/her
library reviewed. This comment was in the context of getting more
participation to the reviews. I thought the comment made sense (it was
"fair") so I participated to 3 reviews before my library was reviewed.
I have to say that such experience really helped me during the review
of my library. I don't know if it'd make sense to make "participating
to 3 reviews" a requirement for a library submission... maybe a
requirement that can be waived only by the review wizards.

> Finally, really big thanks to everyone for participating in the review and
> ancillary discussions. I attempted to be as transparent as possible in
> outlining the rationale for my decision above, but if you have any further
> questions, do not hesitate to ask.
>
> - Jeff, Review Manager for Local

Thank you very much Jeff for managing the review!

--Lorenzo

> [1] http://thread.gmane.org/gmane.comp.lib.boost.devel/168612
> [2] http://lists.boost.org/boost-announce/2008/05/0190.php

Next message: Jeremiah Willcock: "Re: [boost] [graph] LEDA adapter"
Previous message: Martin B.: "Re: [boost] [Serialization] (Commented) assertion when using types in multiple DLLs on Windows (Ticket #3934 and #4394)"

Date view	Thread view	Subject view	Author view

Boost list run by bdawes at acm.org, gregod at cs.rpi.edu, cpdaniel at pacbell.net, john at johnmaddock.co.uk