Re: [scope] review

From: Andrey Semashev (andrey.semashev_at_[hidden])
Date: 2023-12-05 15:54:51

On 12/5/23 17:34, Julien Blanc via Boost wrote:
> 
> While I was not fond of it at the start, i think there's a clear value
> in differentiating scope_success and scope_fail. The first one is
> allowed to throw, the second is not. This, however, is not a strong
> requirement, but a general advice. It thus raises the concern about why
> not simply giving a boolean parameter to the scope_guard handler
> function, and letting it act accordingly. I suggest making the
> noexcept-ness of scope_fail executor a strong requirement.

One of the key points of integrating the "active" flag into the scope
guard is to get rid of the stray boolean variables that previously had
to be used with Boost.ScopeExit and similar facilities. That active flag
is one of the key distinctions between scope_exit/success/fail and
scope_final.

Regarding why scope_fail doesn't enforce noexcept-ness of the action, it
is because the "failure" may be indicated by other means than an
exception. The library explicitly supports error codes as an alternative.

In general, I think that marking scope guards' destructors noexcept is
pointless. If the action throws while there is another exception in
flight, your program will terminate either way. If the action throws
while there is no other exception then why should the scope guard
terminate the program? Throwing in this case might as well be the
intended behavior. Because if it isn't intended then it is the user who
must communicate this by marking his operator() as noexcept.

> scope_success / scope_fail are not default-constructible, although they
> have a valid empty state.

No, they don't. There is an inactive state, but that state is not
"empty", as the function objects in the scope guard remain constructed
regardless of the active/inactive state. In particular, any resources
that may be owned by the function objects remain allocated until the
scope guard is destroyed.

> While there's a rationale behind this (it is a
> bit uncommon to create a scope guard that does nothing), it is a
> limitation for the sake of nothing. Initializing an empty scope_exit
> allows scope escaping (useful when initializing the scope guard in the
> body of an if statement). There will always be the possiblity to do it
> with an optional, so it just seems that forbidding default construction
> has no value.

I think, there is a misunderstanding as to what would be the result of
default-constructing a scope guard.

If default construction is to be supported for scope guards, it would
definitely require both function objects specified in the scope guard
template parameters to be default-constructible as well. Actually, the
condition function would have to be nothrow-default-constructible even.
So default-constructing a scope guard would also default-construct the
function objects, and those function objects *must be callable* in that
state. Note that you can't modify the function objects in the scope
guard after construction. The default-constructed scope guard will be
active, which is consistent with any other scope guard constructor,
where the user doesn't explicitly request otherwise.

Please note that scope guards do not work like std::optional, and adding
support for default construction will not make them work like that.

The use case for default-constructed scope guards, which, presumably,
will only have access to global state and not to locals or `this`, is
very narrow, IMO. However, I might still support it for class types.
(Pointers to functions won't be callable upon default or value
construction.)

> To check whether a scope_xxx is active or not, one has to call the
> active() method. To check whether a unique_resource has a value, one has
> to call the allocated() method. These are additions compared to the TS.
> While they may be useful, the names are inconsistent, both are new, also
> inconsistent with what other nullable types (optional, unique_ptr,
> variant...) uses. While this is common practice accross the c++
> standard, I would not called this *best*. This leads to problems for
> generic code, and proposals such as this one:
> https://herbsutter.com/2022/09/25/something-i-implemented-today-is-void/
> . In both case, i think at least an operator bool() should be provided,
> because it's a more widespread and common way to check for emptiness /
> valid state.

There may be an argument to add `explicit operator bool` to
unique_resource. I didn't do it because it could be confusing if the
resource itself is convertible to bool (e.g. int) and the result of such
conversion is different from testing whether the resource is allocated
(e.g. for POSIX file descriptors or Windows HANDLEs). However,
std::optional has the same problem, and yet it was decided that
`operator bool` is appropriate there. So maybe it is also appropriate
for unique_resource. I was hoping to hear comments on this matter during
the review.

Regarding `explicit operator bool` for scope guards, I don't really see
the reason to add one. As I said before, scope guards are not "nullable"
in any sense.

Regarding interface compatibility between scope guards and
unique_resource, I don't think there needs to be any. I don't think any
generic code would expect to receive either a scope guard or a
unique_resource and do something sensible about it.

> scope_success / fails / exit takes an additional parameter (extension
> over TS), which is the execution condition. scope_success executes when
> the condition returns false. This is unintuitive, and something i will
> usually get wrong.

The function object is called a "failure condition", so when it returns
false it means there's no failure.

> While i get the reasoning behind, i think it tells
> more in favor of a scope_conditional<SuccessHandler, FailureHandler>
> interface. I'm not sure there's a lot of use-cases for a success_only or
> fail_only scope guard, especially since you can release in case of
> success, to avoid the cost of the failure check.

I would say (and I probably have said it before) that there are more use
cases for scope_fail than for scope_success. I can definitely say that I
have found scope_fail very much useful in my practice, so there's that.
I will probably make use of scope_success at some point, as I work on my
code base and refactor it over time.

I'm not sure there's much value in a scope guard taking two function
objects, for success and failure. As I answered to Andrzej Krzemienski,
it is probably better to use scope_exit or scope_final for that purpose
and select between the success/failure branches within the action. This
way, you're avoiding having to bind variables twice.

> About this customizable failure detector, i'm not found of the idea. The
> default, detection of a current exception, is a sane one. And it allows
> us to require FailureHandler to be noexcept. That's no longer the case
> if you allow customizing the failure detector. I'm not sure it's worth
> the tradeoff, since the failure detection could always be done inside
> the body of a standard unconditional scope_exit.

Custom failure condition is useful for supporting error codes. I
consider this an important use case.

> Finally, i don't get the point of set_active. Sure, it's cheap to
> implement, but do we really want to encourage dynamically enabling scope
> guards ? Disabling a scope guard makes a lot of sense, that's what you
> do to disable a rollback scope_guard upon successful operation.
> Dynamically enabling it looks more like being hostile to the reader to
> me. In all cases, it's possible to get this behaviour with a boolean
> inside the scope guard callback, so this use case is already addressed.
> There's no need for it to be a first-class citizen.

I have presented examples for delayed activation of the scope guard in
the docs. It is indispensable if your scope guard needs to be created
(and, consequently, destroyed) at a higher-level scope than where you
decide whether it needs to be activated. That it is missing in the TS is
one of my biggest complaints about it.

> * Is the scope of the library well-defined?
> 
> unique_resource and scope_exit are quite different beasts. They only
> have in common being RAII. But that also include unique_lock,
> unique_ptr, and nobody thought about making them part of the same
> library. I think it is a mistake to mix them.
> 
> unique_resource takes three template parameters, the third one being a
> predicate telling if the resource is valid.

To be clear, the third template parameter is the resource traits, not a
predicate. Resource traits provide two functions:

* make_default, for creating the default value of the resource
* is_allocated, for testing whether the resource is in allocated state

> I understand the wish to
> avoid storing an extra information, when there's a possibility to do
> without by storing an invalid value. But to me, it looks like a more
> widespread problem already solved, for example by
> https://github.com/akrzemi1/markable . It would be far better IMHO to
> get markable in boost, and have unique_resource specialized for
> markable<X> than replicating most of its functionality into
> unique_resource.

I'd like to note that the way resource traits are currently defined,
they allow multiple unallocated states of the resource. For example, for
POSIX file descriptors, any negative value is unallocated while -1 is
just the default. As far as I understand, `markable` doesn't support this.

>From the implementation standpoint, I don't think that relying on
`markable` for detecting unallocated state would simplify the code much,
if at all. You'd still have to effectively duplicate the implementation
of unique_resource.

> - scope_success and scope_fail should not take an execution condition.
> Exception detection should be enough for everyone, or else you should
> resort to scope_exit.

Sorry, I disagree that exceptions should be enough. Why error codes
should be a second class citizen?

> - scope_exit should be default constructible (released by default,
> equivalent to moved-from state). That does not hold for scope_final,
> which is not releasable.

It is not possible to construct an object in a moved-from state. That
is, unless the type provides a special constructor for that purpose,
which a scope guard knows nothing about.