Re: [Boost-users] ScopeExit Review ends tomorrow (Wednesday)

From: Johan Nilsson (r.johan.nilsson_at_[hidden])
Date: 2007-08-22 07:37:31

Jody Hagins wrote:
> The ScopeExit review ends on Wednesday, August 22.  If you want to
> have input on its acceptance into Boost, please provide a review by
> tomorrow.
>

[snip]

> Your comments may be brief or lengthy, but basically the Review
> Manager needs your evaluation of the library.  If you identify
> problems along the way, please note if they are minor, serious, or
> showstoppers.
>

My first review, so bear with me.

> Here are some questions you might want to answer in your review:
>
>    * What is your evaluation of the design?

[I consider the library's interface its design here]

-----
Macros:

- Some people complained about the usage of macros, and even if I (as have
become very popular to say) generally dislike macros there are some things
that cannot be done in any other sensible way. Sure, a combination of a
ScopeGuard class and bind/lambdas is an alternative but that quickly breaks
down when the statements becomes more complex. The proposed ScopeExit
library brings a much more readable alternative.

- [minor] I'm not particularly fond of the usage of PP-sequences, e.g.
BOOST_SCOPE_EXIT((a)(b)(c)) as I believe it uglifies things quite a bit.
Probably controversional, but I'd actually prefer a less general solution as
the number of arguments used is most likely a small one (< 10):

BOOST_SCOPE_EXIT_1 (a) { ...
BOOST_SCOPE_EXIT_2 (a, b) { ...

- [minor] A some one else pointed out, out-of-the-box support for predicates
would be great, e.g.:

BOOST_SCOPE_EXIT_IF_1(pred, a, b) { ...

-----
ScopeGuard-like functionality:

- [serious] I strongly believe that an accompanying ScopeGuard-like class
accepting a function object should be included with the library. Why not
call it "scope_exit", perhaps with support for predicates, e.g.:

using namespace ::boost;

scope_exit alwaysCloseOnExit(
    bind(&File::Close, ref(file))
);

scope_exit closeOnExitIfOpened(
    bind(&File::IsOpen, cref(file)),
    bind(&File::Close, ref(file))
);

-----
Local functions:

- [serious] I've seen the current discussions on local function
declarations. IMHO interesting as they may be, they do not belong in this
library other than as an implementation detail. If local functions are
highly desirable they should go into a separate library, which could be used
for an updated ScopeExit implementation.

----
Optimizations:
- [minor] I do not believe the optimizations should be a documented part of
the library.
>    * What is your evaluation of the implementation?
Haven't studied the code. I've only considered ScopeExit from a user's point
of view, and the implementation details in the docs doesn't set off any
alarms.
>    * What is your evaluation of the documentation?
I found the documentation to be of good quality, modulo some
formulations/spelling errors that I believe other people have already
commented on. Including the implementation part deserves some extra credit.
>    * What is your evaluation of the potential usefulness of the
> library?
I'd say it's somewhere between useful to very useful.
>    * Did you try to use the library?  With what compiler?  Did you
> have any problems?
I did not try to use the library.
>    * How much effort did you put into your evaluation? A glance? A
> quick reading? In-depth study?
I read through the docs and also read most of the replies/discussions in the
mailing list. No in-depth study, but I've put down something in the order of
2-3 hours.
>    * Are you knowledgeable about the problem domain?
Enough so.
>
> And finally, every review should answer this question:
>
>    * Do you think the library should be accepted as a Boost library?
Yes, if the restrictions on defining scope exits in header files on MSVC can
be lifted (which I believe is already fixed?). Very strongly recommending an
accompanying ScopeGuard-like component though.
/ Johan