ScopeExit Review ends tomorrow (Wednesday)
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. BTW, we need more input ;-) Thanks!
From the original announcement...
The review period for Scope Exit Time Series, submitted by Alexander Nasonov, runs from Monday, August 13 until Wednesday, August 22.
From the documentation:
Nowadays, every C++ developer is familiar with RAII technique. It binds resource acquisition and release to initialization and destruction of a variable that holds the resource. But there are times when writing a special class for such variable is not worth the effort. This is when ScopeExit macro comes into play. You put resource acquisition directly in your code and next to it you write a code that releases the resource. Note that ScopeExit uses PP macros and relies on Boost.Typeof. The latest version can is here: http://boost-consulting.com/vault/index.php?action=downloadfile&filename=scope_exit-0.04.tar.gz&directory=& It's available also at http://tinyurl.com/2z6qg2 Online documentation is here: http://194.6.223.221/~nasonov/scope_exit-0.04/libs/scope_exit/doc/html/index...
From the review guidelines here: http://boost.org/more/formal_review_process.htm ...
Boost mailing list members are encouraged to submit Formal Review comments: * Publicly on the mailing list. * Privately to the Review Manager. Private comments to a library submitter may be helpful to her or him, but won't help the Review Manager reach a decision, so the other forms are preferred. What to include in Review Comments 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. Here are some questions you might want to answer in your review: * What is your evaluation of the design? * What is your evaluation of the implementation? * What is your evaluation of the documentation? * What is your evaluation of the potential usefulness of the library? * Did you try to use the library? With what compiler? Did you have any problems? * How much effort did you put into your evaluation? A glance? A quick reading? In-depth study? * Are you knowledgeable about the problem domain? And finally, every review should answer this question: * Do you think the library should be accepted as a Boost library? Be sure to say this explicitly so that your other comments don't obscure your overall opinion. Thanks in advance to every for your participation in this review. _______________________________________________ Boost-users mailing list Boost-users@lists.boost.org http://lists.boost.org/mailman/listinfo.cgi/boost-users
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
"Johan Nilsson" wrote:
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.:
#include
Pavel Vozenilek wrote:
"Johan Nilsson" wrote:
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.:
#include
The ON_BLOCK_EXIT/ON_BLOCK_EXIT_OBJ macros are missing, unfortunately.
I'm aware of the existence of this class. As long as it's only an implementation detail of a library it's not of much help to library users, though.
Joshua Lehrer had wrote a ScopeGuard variant that doesn't care about the no-object - object duality. It can be found at http://www.zete.org/people/jlehrer/scopeguard.html (works OK for me).
Sure, but it would be nice if such a thing would be publicly available in Boost, just using the generalized bind approach. I believe it would fit in nicely within the ScopeExit library, as long as it isn't called "scope_guard" ... / Johan
This is short review of the documentation. I plan to look into it more on VC9, newer versions of GCC and with the AMD profiler. In my opinion the library should be included in Boost (it has the features I asked for and doesn't have the problems I disliked) but the documentation should be more "tired-programmer-with-no-time" friendly. IMO the first page should contain something like this: RAII can be implemented: 1. By creating a special purpose class. Example. 2. By using ScopeGuard (there's already one in Boost hiding deeply in the MultiIndex library - this should be mentioned). 2 examples - one simple, one hideously complicated to show the limitations. 3. By using ScopeExit. 2 examples - one the most simple, the second complex. It should be said that the values are passed by value (and when) and mentioned how to pass them by reference. An example how one can redefine the long macros BOOST_SCOPE_EXIT and BOOST_SCOPE_EXIT_END with something shorter should be provided. Then there should be table listing known C++ compilers and limitations of ScopeExit here: * has it native typeof or emulation? * works with multiple TU? * works in templates? * more efficient version can be used (if it is stays separated, it should be automatic IMHO) * rough estimate of compile time dependency on number of ScopeExits in a TU (this is really important, that's what killed the SMART_ASSERT library). Possibly also effect on linking time (how much of extern stuff does it put into object files). * whether C99 features can be used (possibly every example could have C++98 and C99 variant) Ideally there would be link to example for every compiler in the table showing what fails and what works. This first page should be sufficient enough for 90% of users. Other pages: Every other macro mentioned in the documentation should start with information who may be interested in it and for who it is irrelevant. This information should be visually separated from the rest of the text. If possible the "experimental" macros should be removed from the documentation - this word just sends a cold chill down my neck and could easily discourage people from using the library. Whether the concept is useful: In my opinion yes because: a) Such a feature was added into the D language and I know one proprietary scripting language which implemented it as well and it had helped to find a few hard to notice bugs. b) The ability to put cleanup code near to related code is invaluable. Cleanup code gets rarely tested (or may be practically untestable) and every bit which helps to spot a bug is invaluable. c) The library helps to reduce number of nested blocks, one of the most error prone construct. d) The library could be also very useful for those who maintain or gradually rewrite a large legacy codebase since it allows minimal and local changes. Whether it is good to add yet another macro-library to Boost: * Well, that's the way it works. Perhaps C++1x will get native on_exit { } construct. The current library allows to put a breakpoint into the code which makes more debugeable code than, say, one using std algorithms. The information whether local variables from the macro list will show up in the debugger may be put somewhere in the docs. ------------------- I'll take deeper look on the code later. I wrote this short review to catch up the end of review period. -------------------- The documentation may also mention alternative ScopeGuard implementation in http://tnfox.sourceforge.net/TnFOX-0.87/html/df/d4f/group__rollbacks.html can be mentioned in passing. It features so called "group undo" for multiple ScopeGuards, providing something like transactions. /Pavel
"Pavel Vozenilek" wrote:
It should be said that the values are passed by value (and when) and mentioned how to pass them by reference.
Eh, vice versa. Typed too fast. ----------------------- I'll add my own experience to the discussion of macro heresy: I prefere BOOST_FOREACH instead of std algos + bind + lambda because: * I know what I wrote. With algos/bind/lambda it is always great thrill and one feels like a discoverer of a new continent when it gets finally right but at the end it is net waste of my time. * I can debug it, easily. I'd say both will apply on ScopeExit versus a complex ScopeGuard. Macros can be more practical than other constructs, sometimes. /Pavel
participants (3)
-
Jody Hagins -
Johan Nilsson -
Pavel Vozenilek