Re: [boost] [scope] review

wt., 5 gru 2023 o 16:55 Andrey Semashev via Boost 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 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... 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