index.html: "That it." should be "That's it." tutorial.html: "Had C++ variadic macros introduced in C99, arguments could have been passed more traditionally BOOST_SCOPE_EXIT(commit, m_persons). Passing Boost.Preprocessor sequence makes possible to bypass this C++ limitation." Should be "If C++ had variadic macros, introduced in C99, arguments could have been passed more traditionally--BOOST_SCOPE_EXIT(commit, m_persons). Passing a Boost.Preprocessor sequence bypasses this C++ limitation." "A body of a ScopeExit block is executed at the end of scope of the block." Should be "The body of a ScopeExit block is executed at the end of scope of the block." "In case of normal execution flow, the commit is true and code inside the block does nothing." Should be "In case of normal execution flow, commit is true and the code inside the block does nothing." "The m_persons is also passed and it should be registered too." Should be "m_persons is also passed and should be registered too." "If the first step m_persons.push_back(person) throws, the m_persons object doesn't change and the addPerson function has no effect at all. Note that the person object cannot be added to an arbitrary position because in failure case the object should be erased from a middle of the m_persons but if copying of any Person object after the object being erased throws, the m_persons is left in inconsistent state." Should be "If the first step, m_persons.push_back(person), throws, the m_persons object doesn't change and the addPerson function has no effect at all. Note that the person object cannot be added to an arbitrary position because in case of failure the object should be erased from the middle of m_persons, but if copying of any Person object after the object being erased throws, m_persons is left in inconsistent state." "BOOST_SCOPE_EXIT macro never catches exceptions. If it did, you could end up with inconsistent state because some block couldn't execute completely. Though, catching exceptions may be useful in some cases. The library provides a pair of macros BOOST_SCOPE_EXIT_TRY and BOOST_SCOPE_EXIT_CATCH_ALL." Should be "The BOOST_SCOPE_EXIT macro never catches exceptions. If it did, you could end up with inconsistent state because some blocks couldn't execute completely. Catching exceptions, however, may be useful in some cases. The library provides a pair of macros BOOST_SCOPE_EXIT_TRY and BOOST_SCOPE_EXIT_CATCH_ALL." Also, I think a more important reason for not catching exceptions is that the block ought not to do anything that could throw in the first place. If I need to put a catch() in I want it to be explicit. That way if I accidentally throw I will get an obvious error rather than a silent failure that may not manifest itself until a lot later when tracing its cause is much more difficult. "Below is a complete body of the addPerson. It's quite big yet easy to read. It consists of four steps, each starts with an action followed by a rollback action inside ScopeExit body. Bodies are executed in reverse order. The final step is commit." Should be "Below is the complete body of addPerson. It's quite big yet easy to read. It consists of four steps. Each starts with an action followed by a rollback action inside the ScopeExit body. Bodies are executed in reverse order. The final step is commit." alternatives.html: "An instance of File doesn't close a file in a destructor, a programmer is expected to call the close member function explicitly." Should be "An instance of File doesn't close a file in a destructor. A programmer is expected to call the close member function explicitly." "Though, RAII is good for short transactions like this one:" Should be "RAII, however, is good for short transactions like this one:" * lambda expressions are hard to write correctly, for example, overloaded function must be explicitly casted, as demonstrated in this example, * forgetting to pass commit by reference may lead to a subtle bug, * condition in if_ expression refers to commit variable indirectly through _1 placeholder, * setting breakpoint inside if_[ ... ] requires in-depth knowledge of Boost.Lambda and debugging techniques. Should be * lambda expressions are hard to write correctly. For example, an overloaded function must be explicitly casted, as demonstrated in this example * forgetting to pass commit by reference may lead to a subtle bug. * the condition in the if_ expression refers to the commit variable indirectly through the _1 placeholder. * setting a breakpoint inside if_[ ... ] requires in-depth knowledge of Boost.Lambda and debugging techniques. reference.html "Each identifier in scope-exit-args-pp-seq must be a valid name in enclosing scope." Should be "Each identifier in scope-exit-args-pp-seq must be a valid name in the enclosing scope." "The ScopeExit uses Boost.Typeof to determine types of scope-exit-args-pp-seq elements." Should be "ScopeExit uses Boost.Typeof to determine the types of scope-exit-args-pp-seq elements." "Although BOOST_SCOPE_EXIT_TPL has the same suffix as the BOOST_TYPEOF_TPL, it doesn't follow a convention of the Boost.Typeof." Should be "Although BOOST_SCOPE_EXIT_TPL has the same suffix as BOOST_TYPEOF_TPL, it doesn't follow the convention of Boost.Typeof." impl.html: "but some requirements made implementation a bit more complicated:" Should be "but some requirements made the implementation a bit more complicated:" "Doing this would change a code starting from a definition of the scope_exit_t_1 struct in the following way:" Should be "Doing this would change the code starting from the definition of the scope_exit_t_1 struct to the following:" In Christ, Steven Watanabe

* Are you knowledgeable about the problem domain?

I frequently use the RAII pattern in day-to-day programming, and I often advocate the use of this pattern as one of the major highlights of the C++ language.

* What is your evaluation of the potential usefulness of the library?

Boost has needed a library like this for a long time. I believe a library that does something similar to this should be one of the core language support libraries that all C++ programmers should use.

* What is your evaluation of the design?

My main concern here is that this library only provides a macro-ized version, and nothing analogous to Alexandrescu's ScopeGuard, which takes a functor. I think this is a serious problem. The documentation states, "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." For very small amounts of exit code, which is usually the case in my experience, a BLL lambda expression is perfectly suitable. The most common action to be taken, in my experience, is just a function call, which in fact only needs bind. The main reasons for my preference of ScopeGuard over the proposed interface for small exit code sequences are: -Lower ugliness factor. -Better cohesiveness with possibly more complicated code patterns. -Easier analysis by automated tools. RECOMMENDATION 1: Something comparable to ScopeGuard or the detail scope_guard should be included in this library. A secondary concern is that about 2/3 of the time I'm using RAII, the pattern is a "conditional free" of the type explicated in the documentation example:

if(!commit) m_persons.pop_back();

The pattern here is: 1) Initialize a variable. 2) Do some work. 3) Set a variable. 4) Free a resource if variable is not set. This process is tedious and error-prone, and very much distracting from the main work to be done. Minimizing the amount of distracting error code necessary is one of the most important jobs of the code writer. I understand that it is impossible to determine whether normal exit or unwinding has triggered a destructor, but I believe there may be other ways to encode this pattern, at least freeing the implementor from having to type an explicit 'if.' RECOMMENDATION 2: Support for the "conditional free" pattern should be added.

BOOST_SCOPE_EXIT( (commit)(m_persons) )

This construct is confusing to me. I believe this is a PP sequence, but I'm not sure. Since this library should be used in all parts of code, and not only by library implementors, I do not think it is appropriate for programmers to be exposed to the PP metaprogramming syntax. Normal function call notation should be used. RECOMMENDATION 3: Use familiar syntax.

* What is your evaluation of the documentation?

It needs to be expanded. The exact semantics of this library are very unclear to me. Each identifier should be formally specified, and the overall semantics explained. For example: -The parameter of BOOST_SCOPE_EXIT is not explained. -What constructs are permitted as parameters to the above macro? How many times are they evaluated? -Exception-safety guarantees, if any, are not explained. What happens if an exception is thrown from within the exit code? Also, there needs to be analysis regarding performance and storage efficiency impact, if any, as well as any other similar considerations. RECOMMENDATION 4: Expand documentation.

* 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.

This library would be a valuable addition, but it is incomplete in its present form. As it stands now, I probably would not use it, for the reasons stated above. However, the "long exit code" case (where both a specialized class and lambda expressions are inappropriate) does come up from time to time, so I might consider using it in those cases. I do not believe this library should be accepted until it has been expanded and re-reviewed with these use features and more complete documentation.

AMDG I vote to accept ScopeExit into boost. As has already been mentioned you need to use __COUNT__ instead of __LINE__ for msvc. BOOST_SCOPE_EXIT_FASTER_IMPL is slower under msvc 8.0 #include <iostream> #include <boost/scope_exit.hpp> #include <boost/timer.hpp> int main() { boost::timer timer; for(int i = 0; i < 100000000; ++i) { int j = 0; BOOST_SCOPE_EXIT((j)) { ++j; } BOOST_SCOPE_EXIT_END; } std::cout << timer.elapsed() << std::endl; } This takes .718 seconds with BOOST_SCOPE_EXIT_FASTER_IMPL and .484 seconds without it. I think that you should remove this option since it doesn't do any good. BOOST_SCOPE_EXIT_TRY/BOOST_SCOPE_EXIT_CATCH_ALL: I do not think that these macros are useful. If I want to catch all the exceptions then I can just put try/catch in myself. There is no need to have an extra pair of macros to do it for me. boost_scope_exit_params_struct_nn needs to declare an assignment operator to suppress warning C4512: 'main::boost_scope_exit_params_struct_20' : assignment operator could not be generated. In Christ, Steven Watanabe

Hello, I don't have time for the full review, sorry. Just one bug report to help you: // Step 2: append the person to the m_persons. m_persons.push_back(person); BOOST_SCOPE_EXIT( (commit)(m_persons) ) { if(!commit) m_persons.pop_back(); } BOOST_SCOPE_EXIT_END The standard in section 23.1 "Container requirements" says that: "if an exception is thrown by a push_back() or push_front() function, that function has no effects" it means that the code above is buggy - in a case of exception in a copy constructor of Person it removes one element from a container. In other words, if before push_back m_persons.size() was 1, after scope_exit body execution m_persons.size() == 0. I'm not sure if I can vote with such a little effort involved, but if I could, I vote to accept this library into boost. Best regards, Oleg Abrosimov

Hello, I haven't participated in a boost review before, but I feel I have enough experience with this topic to throw in my two cents. I have worked with scope guard (or scope guard like) objects for the last 7 years and have very recently started replacing our custom implementation with the internal version found in the boost multi index container library (due to the lack of an official one). I consider the scope guard to be a well-known and well-documented idiom and I fail to see what the ScopeExit library adds to it (or fail to be convinced by the arguments made in the alternative section of the help). Most programmers have by now come across some implementation of this idiom and are familiar with the RAII techniques that give the guard its clever functionality. Specifically because of this, I think it would be foolish to accept an implementation into boost that differs from everything that has been published out there. Rolling this functionality into a macro that makes the C++ syntax similar to D seems rather pointless and only contributes to a very unnatural C++ syntax. As far as I am concerned, macros should be avoided at all times unless there are no good alternatives and in this case, I think there are. I truly appreciate the attempt to (finally officially) add this functionality into boost but I would much rather see a plain scope guard implementation (with or without a macro for unnamed instances) up for review. I vote against this library. -Ben This e-mail and any attachment is for authorised use by the intended recipient(s) only. It may contain proprietary material, confidential information and/or be subject to legal privilege. It should not be copied, disclosed to, retained or used by, any other party. If you are not an intended recipient then please promptly delete this e-mail and any attachment and all copies and inform the sender. Thank you.

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

I vote to accept BOOST_SCOPE_EXIT. The library complements scope guard in the same way as BOOST_FOREACH complements std::for_each. Both of them has a place, and it depends on the users preferences what they will use. I prefer BOOST_SCOPE_EXIT. I would wish for a syntax more in line with BOOST_FOREACH (omitting the BOOST_SCOPE_EXIT_END, but the best I've been able to come up with is: {BOOST_SCOPE_EXIT( (commit)(m_persons) ) { if(!commit) m_persons.pop_back(); } }; With the leading open clause, this is not exactly an improvement... I too miss the BOOST_LOCAL_FUNCTION_DEF macro. This should be a part of the library before it is included in boost. One possible implementation is: int i=5; int j=4; int BOOST_LOCAL_FUNCTION_DEF(name,(i)(j))(int x,int y) { i=x;j=y; return i+j; } BOOST_LOCAL_FUNCTION_DEF_END //... name(8,6) which expands to: int i=5; int j=4; int (*function_ref)()=0; typedef BOOST_TYPEOF(i) type_0; typedef BOOST_TYPEOF(j) type_1; typedef BOOST_TYPEOF(function_ref()) return_type_1; struct name_impl { type_0& i; type_1& j; name_impl(type_0& i_,type_1& j_) : i(i_),j(j_) {} return_type_1 operator()(int x,int y) { i=x;j=y; return i+j; } } name(i,j); //... name(5,6); When it comes to handle references or copies of arguments, I think the following can be made to work: int BOOST_LOCAL_FUNCTION_DEF(name,ref(i)const_ref(j)(k))(int x,int y) If not, the following can be made to work by using BOOST_PP_IS_UNARY. ( (const&)(i) ) ( j ), but this is starting to look like lisp. Regards, Peder

Thanks for all the input on Alexander's ScopeExit submission. There are some good discussions, so please continue them over the next day or so if necessary. I am very busy this weekend (I am HC of a high school football team, and our first game is tomorrow night), but I will render a conclusion some time next week. Again, thanks for the input.

The discussion threads included comments from several people who did not offer an actual review and/or YES/NO. I have filtered every message with either scope or exit in the subject, and I want to make sure I have all the reviews. If your name is in this list, that means I have a YES/NO review from you. Otherwise, I may see some discussion from you, but no YES/NO review. If your name is NOT in this list, but you feel you provided a review, please point me to your review posting, or provide appropriate clarification. The fact that you participated in the discussion does not mean that I know your opinion on the acceptance of ScopeExit. In addition, I have received a few reviews via private email. For the sake of fairness and openness, I asked the authors to send their review to the list. I will not consider them unless they appear in the open mailing list. Thanks for your cooperation! Kim Barrett Aaron LaFramboise Steven Watanabe Ben Kuppers Andrey Semashev Peder Holt Goran Mitrovic Johan Nilsson Pavel Vozenilek Tom Brinkman Maxim Yanchenko