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