Hello Jody, Here's my review of the library. Sorry for the lengthy post.
* What is your evaluation of the design?
Overall library design is good. I'd like to state some notes that look discouraging to me: - The usage of Boost.Typeof may be discouraging because of need to register user-defined types. This might be no problem for types that indeed are defined by the user, but there are cases when they are not (OS API, third party libraries, etc.). And interaction with such APIs in a transaction-like manner is a quite probable place to employ ScopeExit. - The scope-exit block arguments are taken by reference. I consider this as a drawback because it is most often desirable to fix the environment state at the point where the guard is created. The most frequent and, in majority, the only exception to this is the "commit/dispose" flag. Playing otherwise may lead to subtle errors. Consider the following examples: Example 1: void foo(std::set< int >& my_set) { bool commit = false; std::set< int >::iterator it = my_set.insert(10); BOOST_SCOPE_EXIT((my_set)(it)(commit)) { // Which one will be erased here? // Not necessarily the one we intended to. if (!commit) my_set.erase(it); } BOOST_SCOPE_EXIT_END // Do smth. with "it" it = my_set.find(5); // and reuse it // Do smth. with "it" again commit = true; } Example 2: class A { int m_i; void foo() { m_i = 1; BOOST_SCOPE_EXIT((m_i)) { // What's the value of m_i here? std::cout << m_i << std::endl; } BOOST_SCOPE_EXIT_END bar(); } // Assume that bar's implementation is somewhere far away void bar(); }; These are just illustrations, I understand that I could have created copies of the needed variables on the stack. But I think this should be the default behavior of the library, and passing by reference should be an available option. That said, I'd like to vote to add a new set of macros that would allow: - Explicitly state the scope-exit argument types - Explicitly state if arguments should be bound by reference or not These macros should not depend on Boost.Typeof in any way (not even #include it). The syntax might involve PP tuples like this: BOOST_SCOPE_EXIT_BEGIN( (int, i)(BOOST_TYPEOF(x + y), j)(bool&, commit)) { } BOOST_SCOPE_EXIT_END Variables i and j are bound by value and commit - by reference in the snippet above.
* What is your evaluation of the implementation?
I didn't inspect it much. __LINE__ macro bug on VC 7.0+ has already been noted by Steven.
* What is your evaluation of the documentation?
The documentation looks quite sufficient to me, although I have some notes (not sure if it's already been reported, sorry for the noise if so): - Configuration page. It says that BOOST_SCOPE_EXIT_FASTER_IMPL is not supported which makes me feel like I'm discouraged from using it. May be it shouldn't be documented then? - If BOOST_SCOPE_EXIT_FASTER_IMPL optimizes something, it would be nice to see performance comparisons to the non-optimized version. - Anyway, performance section would be appreciated. I'd like to see the speed impact comparing to other alternatives you listed in the docs. In particular, I'd like to see the difference depending on the scope-exit block arguments number. I can see some performance notes on the Implementation page, but there are no experimental data. - Introduction, in the bottom: "That it.". Should be "That's it.".
* What is your evaluation of the potential usefulness of the library?
I think, the library is very useful in its domain and will help to write exception-safe code in the majority of cases. Although I believe that it might benefit from my suggestions above.
* Did you try to use the library? With what compiler? Did you have any problems?
Tried to compile and play around with examples on VC 8. No problems encountered.
* How much effort did you put into your evaluation? A glance? A quick reading? In-depth study?
I've read the docs and tried the examples.
* Are you knowledgeable about the problem domain?
I am. I think, most of us are. :)
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.
I think, the library is worth adding to Boost. It elegantly solves an every-day problem of writing exception-safe code. -- Best regards, Andrey mailto:andysem@mail.ru