Re: [boost] ScopeExit mini-review

From: Alexander Nasonov (alnsn_at_[hidden])
Date: 2007-08-14 05:07:58

Aaron W. LaFramboise <aaronrabiddog51 <at> aaronwl.com> writes:

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

You can always emulate ScopeGuard:

boost::function<void()> on_exit =
    if_(currency_rate_inserted && !var(commit)) [
        bind(
            static_cast<
                std::map<std::string,double>::size_type 
(std::map<std::string,double>::*)(std::string const&)
            >(&std::map<std::string,double>::erase)
          , &rates
          , currency
          )
        ];

BOOST_SCOPE_EXIT( (on_exit) ) { on_exit(); } BOOST_SCOPE_EXIT_END

but it's usually easier to write

BOOST_SCOPE_EXIT( (currency_rate_inserted)(commit)(rates)(currency) )
{
    if(currency_rate_inserted && !commit)
        rates.erase(currency);
} BOOST_SCOPE_EXIT_END

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

I suspect that many people has a fear of using Boost.lambda and they're right
because they may end up writing the above lambda expression.

> The most common action to be taken, in my experience, is just a function 
> call, which in fact only needs bind.

Conditional call is quite common too. But even an unconditional call to
overloaded function is painful. See above example.

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

The example above proves the opposite.

> -Easier analysis by automated tools.

Can you give an example?

> RECOMMENDATION 1: Something comparable to ScopeGuard or the detail 
> scope_guard should be included in this library.

I don't see why it should be included.

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

main work != main flow.
Error handling is as important as a main flow of execution.
I don't see how ScopeExit is worst than ScopeGuard in distracting from
the main flow. try/catch and try/finally are even worst, see the Alternatives
section.

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

How?

> RECOMMENDATION 2: Support for the "conditional free" pattern should be 
> added.

Adding it would make the library more complex. Simple "if" inside a block
would do this.

>  > BOOST_SCOPE_EXIT( (commit)(m_persons) )
> 
> This construct is confusing to me.  I believe this is a PP sequence, but 
> I'm not sure.

In tutorial: 'One argument is passed to BOOST_SCOPE_EXIT. It's
Boost.Preprocessor sequence of non-global variables that can be used
inside the block.' 

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

See the first note in tutorial. After proof-reading by Steven Watanabe
it 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."

> RECOMMENDATION 3: Use familiar syntax.

I wish I could.

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

Tutorial:
"One argument is passed to BOOST_SCOPE_EXIT. It's Boost.Preprocessor sequence
of non-global variables that can be used inside the block. Global variables
can be used even if they are not listed. Variables are always passed by 
reference."

Reference:
"The ScopeExit schedules an execution of scope-exit-body at the end of the 
current
scope. The scope-exit-body blocks are executed in the reverse order of ScopeExit
declarations in the given scope.

Each identifier in scope-exit-args-pp-seq must be a valid name in enclosing 
scope.
These names are passed by reference to scope-exit-body and appear in scope-exit-
body
under the same names. The scope-exit-args-pp-seq must not contain duplicates. 
Only
identifiers listed in scope-exit-args-pp-seq, static variables, extern variables
and functions, and enumerations from the enclosing scope can be used inside
the scope-exit-body."

> -What constructs are permitted as parameters to the above macro?  How 
> many times are they evaluated?

Tutorial:
... Boost.Preprocessor sequence of non-global _variables_ ...

Reference:
Each _identifier_ in scope-exit-args-pp-seq must be a valid _name_ in enclosing 
scope.

> -Exception-safety guarantees, if any, are not explained.  What happens 
> if an exception is thrown from within the exit code?

Tutorial: "BOOST_SCOPE_EXIT macro never catches exceptions".

> Also, there needs to be analysis regarding performance and storage 
> efficiency impact, if any, as well as any other similar considerations.

In the Implementation section:
"Total run-time cost of this code is few assignments and an indirect function
call. It should be good enough for most cases."

--
Alexander