[scope] some more thoughts

From: Andrzej Krzemienski (akrzemi1_at_[hidden])
Date: 2023-12-03 13:50:09

Hi Everyone,
I want to share some thoughts regarding the review of Boost.Scope.
The fact that it wants to implement 1-to-1 a feature from a TS -- not from
the standard -- impedes the review process.
The reviewer says, "this feature is bad". The author responds, "but this is
how the TS requires it to be". And now what? Should we accept a bad feature
because the TS requires so? Being TS-conformant is very constraining for
the author. Maybe this is not a worthy goal. Also, my observation from
recent years is that putting stuff into a TS may be a polite way of saying,
"we respect you, but we do not want this."

Also, the goal of having an implementation for a proposed feature is to
check if the interface has been battle-tested to be sound. The Boost Review
process is such a test. When the feedback is, this feature is wrong, then
this feedback should affect the STD proposal. But Boost shouldn't be
compromised by an STD proposal that was submitted without a battle-tested
implementation.

The move assignment operator in scope_exit and friends. It looks like its
existence is against the goal of the feature. You define a guard and then
you use it. Why would you assign a different guard to it?

Why do we have scope_success? An alternative to using scope success is to
just put your instructions at the end of the function. Do you have multiple
return statements in your function and at the same time you need the same
sequence of operations to be performed at each end? This is a good
motivation to restructure your function to have a single return. Multiple
returns are generally OK, but needing the same sequence warrants a single
return.
scope_success seems like a solution that is looking for a problem.
"But the TS has scope_success" -- this is not a good argument for me. In
Boost Review we should be evaluating a library based on its own merit, not
based on a similarity with some other design, which can have its own flaws.

Mixing unique_resource with scope guards is not a good idea. The STD tends
to use big aggregate headers. Boost does not. The  solution from the TS
should not be ported 1-to-1 to Boost.

Boost.Scope offers at least three ways to do the same thing. For instance,
to execute some code only upon some inconvenient condition, I can use
scope_fail with my condition, or scope_exit with my condition, or I can add
set_active on top of either. I consider this to be a bad design. Source
code is not only for the flexibility of writing, but also for the ease of
reading and learning. Having multiple ways to do the same thing impedes the
understanding of the code.

While I understand the existence of the triplet `scope_exit`, `scope_fail`
and `scope_success` (D uses an analogous triplet), it takes a while to
understand the difference between scope_exit and scope_final. And the
choice of name doesn't help there. The Standard library chose to use term
guard (std::lock_guard) to indicate something small and cheap. Maybe use
this name instead. Not perfect either, but at least consistent with what we
have in STD.

Also, my experience is that one uses scope guards only in rare situations
where you need to satisfy complex guarantees, like maintaining the same
size of two vectors. And in those situations one is already very cautious,
and tries to structure the code for the minimum surprises. One does not use
multiple returns or complex control flows.
The justification I heard for so many features in Boost.Scope is that there
may be complex situations that require it. I think that in these situations
restructuring the code is better than adding more features to Boost.Scope.

This looks like a lot of harsh words, but I find a subset of the library
very useful:
* the unconditional execution of the function
* the execution based on manual deactivation (active being the initial
state)
* execution based on the uncaught exception count, with the caveat that it
sometimes doesn't work, and it is not cheap.

Regards,
&rzej;