Re: [boost] ScopeExit Review Begins Today

From: Andrey Semashev (andysem_at_[hidden])
Date: 2007-08-21 15:05:05

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_at_[hidden]