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