Re: [scope] The comparison with Boost.ScopeExit is unfair

From: Andrey Semashev (andrey.semashev_at_[hidden])
Date: 2023-12-02 16:10:41

On 12/2/23 17:14, Andrzej Krzemienski wrote:
> pt., 1 gru 2023 o 19:49Â Andrey Semashev via Boost <boost_at_[hidden]
> 
>     Second, if you mean incompatibility of exception_checker with coroutines
>     then that is already addressed in a note in this section:
> 
>     https://lastique.github.io/scope/libs/scope/doc/html/scope/scope_guards.html#scope.scope_guards.condition_functions <https://lastique.github.io/scope/libs/scope/doc/html/scope/scope_guards.html#scope.scope_guards.condition_functions>
> 
>     If you're using coroutines then just don't use it. Where it is used in
>     library examples, it is implied that coroutines are not used.
> 
> I mean something slightly different.
> 
> Given that in Boost.ScopeExit I need to implement the conditional
> execution manually, I can do it in two ways. One is:
> 
> template< typename T >
> void add_object(T const& arg)
> {
> Â  Â  auto it = objects.insert(Object());
> 
> Â  Â  auto uncaught_count = boost::core::uncaught_exceptions();
> Â  Â  Â  Â 
> Â  Â  BOOST_SCOPE_EXIT_TPL(this_, it, uncaught_count)
> Â  Â  {
> Â  Â  Â  Â  if (uncaught_count != boost::core::uncaught_exceptions())
> Â  Â  Â  Â  Â  Â  this_->objects.erase(it);
> Â  Â  }
> Â  Â  BOOST_SCOPE_EXIT_END;
> 
> Â  Â  it->start(arg);
> }
> 
> The other is:
> 
> template< typename T >
> void add_object(T const& arg)
> {
> Â  Â  auto it = objects.insert(Object());
> 
> Â  Â  bool active = true;
> Â  Â  Â  Â 
> Â  Â  BOOST_SCOPE_EXIT_TPL(this_, it, &active)
> Â  Â  {
> Â  Â  Â  Â  if (active)
> Â  Â  Â  Â  Â  Â  this_->objects.erase(it);
> Â  Â  }
> Â  Â  BOOST_SCOPE_EXIT_END;
> 
> Â  Â  it->start(arg);
> Â  Â  active = false;
> }
> 
> Given these options, I have zero motivation for using the former. The
> latter one:
> * is C++11 compatible (boost::uncaught_exceptions doesn't necessarily
> work for prec++17 compilers)
> * works correctly in 100% cases rather than in 99% of the cases (I mean
> the caveats regarding coroutines and fibers)
> * avoids two calls to a C++ runtime.
> 
> I think the example with the `active` flag should be used in comparison.
> And then, if you want to compare apples to apples, you would have to use
> scope_exit and set_active. Otherwise, I think the comparison should
> acknowledge that the solution with scope_fail has caveats and is slower.

We've discussed this before, and there are pros and cons to each
approach. The uncaught_exceptions-based solution encapsulates the
rollback logic in one place, you don't have to set the active flag
manually on every return point. It does have downsides as well, like
higher performance cost (unless the compiler implements it using an
intrinsic) and incompatibility with coroutines (which might not matter
if you don't use or care about coroutines). So neither one is wrong or
bad, it all depends on your requirements. Personally, I have used the
solution based on uncaught_exceptions quite a few times back when I was
using Boost.ScopeExit, for the reasons I mentioned.

Back to the table in the docs, there are two main points I'd like to
make. First, this is an Introduction section. It is not meant to be an
exhaustive description of library features and caveats. It only serves
to give the reader the basic understanding of what the library is about
and a glimpse of how it might look in his code. Hence I do not think
that it is the right place to dive into details and corner cases, there
are other sections in the docs for that.

Second, the reason I'm giving the uncaught_exceptions-based example in
the table, besides that this is the kind of code I actually wrote and
eventually converted to Boost.Scope, is that the
uncaught_exceptions-based version of the Boost.ScopeExit code is the
exact equivalent of the Boost.Scope example that uses scope_fail. IOW,
this is apples-to-apples comparison, as opposed to using different
solutions for different libraries. I think, this is only fair.

You may argue to include a row of examples, where Boost.ScopeExit code
uses the `active` flag. I suppose, I could do this, although to a great
degree it would duplicate the row that already exists. I'm not sure how
useful it would be, as it would considerably increase the size of the
section for little value.