Re: [scope] review

From: Andrzej Krzemienski (akrzemi1_at_[hidden])
Date: 2023-12-07 15:21:18

wt., 5 gru 2023 o 16:55 Andrey Semashev via Boost <boost_at_[hidden]>
napisał(a):

> 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.
>

I Agree. The library should on the other hand make it clear to the users
that it calls the user-provided function in destructor, and that whatever
precautions they take for their destructors, they should also apply them to
callbacks passed to scope guards.

>
> > 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.
>

Boost.Scope got that right.
I think that it would be a bad approach to design "I'll do it because I
can, even though there is no use case for it, and it is dangerous".

>
> > 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.)
>

There is a value in not offering in the interface something that can easily
lead to bugs, and brings minimum value (users can get the desired effect
with optional).
I support the current design with no default constructors.
Ideally I would see the move constructors removed, but I understand it is
impossible before C++17.

>
> > 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.
>

Oh, it does. The docs show one such example in
https://github.com/akrzemi1/markable/blob/master/documentation/overview.adoc#defining-a-custom-marked-value-policy
You have to indicate one of the "marked" stats as special, as it will be
the one that the library will set on your behalf. But other than that you
can define a predicate that tells marked states from normal states.

>
> 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.
>

That might still be true. It would also introduce a dependency.
But brevity would not be the only goal. Another would be to indicate the
common pattern.
But Markable is not in Boost, so this is moot.

Regards,
&rzej;

>
> > - 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.
>
>
> _______________________________________________
> Unsubscribe & other changes:
> http://lists.boost.org/mailman/listinfo.cgi/boost
>