[boost] ScopeExit mini-review

From: Aaron W. LaFramboise (aaronrabiddog51_at_[hidden])
Date: 2007-08-13 17:09:58

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