Hi, This is my review of the proposed boost scope library. * Does this library bring real benefit to C++ developers for real world use-case? Yes. Scope guards are a well-known and widely used feature, nearly every code-base has some implementation of it. The same goes for raii resource wrappers. * Do you have an application for this library? I would consider using it for new projects. It does not, however, offer significant value at the time which would justify migrating from our homebrew scope guard. Worse, it misses some guarantees that makes it less fitted (but arguably it is more generic than what we use now). * Does the API match with current best practices? The API is copied from a TS proposal, for some parts. Most of my griefs against the API also applies to the TS. 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. scope_success / scope_fail are not default-constructible, although they have a valid empty state. 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. 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. 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. 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. 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. 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 like the idea of the lighter, unconditional, scope_final, which is missing in the TS. * 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. 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. * Is the documentation helpful and clear? The documentation is clear enough, although i could not find some information that i think should be emphasized more (ie, what the library guarantees in terms of pre / post conditions, noexceptness, etc). I don't like the way the reference section subitems are links to the file headers descriptions, and not links to class descriptions. I'd prefer something like: Reference * template< > class scope_exit * template< > class scope_final * ... instead of the headers, for which i usually don't care (i just need to know which header i need to include, and this information is provided in the class description). There are a few examples in the documentation that can be improved. There's always room for improvement, the overall quality is good. * Did you try to use it? What problems or surprises did you encounter? How much time did you spent? I made some small experiments with it. I spent around 6-8 hours reviewing the library and writing this review. Most of the time was spent reading the documentation and checking the behaviour. No problems and no surprises so far. * What is your evaluation of the implementation? The library is big (~2,5k lines of code), given what it offers. Most of this comes from the fact that the library offers a lot of customization points, maybe too much. Others have pointed that it reimplements some standard features, for the sake of not including some headers. I don't like this choice, but I don't know what's boost policy in this regard is. In all cases, a reimplementation with a name is better than nothing. In particular, there are some static_cast<T&&> which would advantagely be replaced by a detail::forward<T>, the latter being far more readable than the former. It's a shame that compact_storage is not provided as a facility: i guess there are several similar implementation across boost. But that's not the library's fault. Outside that, the organisation is clean, and the code largely readable, given that it needs to be compatible with C++11. * What is you final evaluation of the library? My current recommandation is to CONDITIONNALY ACCEPT the library. I think the following should be adressed, but i'd note that several of these comments also applies to the TS: - scope_success and scope_fail should be unified into a single call, taking two callbacks. This will make a slightly bigger object, but if you plan on detecting exceptions then you don't mind the extra cost anyway. And it will allow to evaluate the condition only once, so that may prove a gain in several situations. - 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. - the failure handler (scope_fail) should be enforced to be noexcept - 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. - the active / released name fiasco should be adressed. At least provide operator bool(). - set_active should be removed. This is encouraging bad practice. - unique_resource should be moved outside the library. I consider that it belongs to a separate library, and i think it has some flaws in its design that should be addressed. Finally, many thanks to Andrey for proposing this library, and for quickly replying to the questions during the review. Regards, Julien
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.
Le 2023-12-05 16:54, Andrey Semashev via Boost a écrit :
On 12/5/23 17:34, Julien Blanc via Boost wrote:
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 think there's a misunderstanding here. My point was exactly about asserting is_nothrow_invocable for the scope_fail constructor argument, not for its destructor.
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.
They indeed have two inactive states: * the released() state * the moved-from state Default construction would be similar to a moved-from state.
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.
That's not exact. the default condition function is always_true, and it is nothrow-default-constructible. So it all boils down to the function the user provide. By default creating to the inactive state, you don't need it to be invocable in its default-constructed state. But i found the root of the misunderstanding: there's no move operator in scope_exit. We have one in our scope guard, I incorrectly assumed it was also present. Without it, default construction does indeed not lead to something very useful.
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.
Ok, granted. In our code base, this is exactly why we have a move assignment operator for scope_guard. I find this solution more elegant, but it requires you to have a type-erased trivially copyable functor. [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.
It does, and in a way that is very similar to unique_resource: https://github.com/akrzemi1/markable/blob/master/documentation/overview.adoc...
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.
A very basic implementation of unique_resource could ressemble this: template<typename Resource, class Deleter> class unique_resource { std::optional<Resource> r_; Deleter d_; public: unique_resource(unique_resource const&) = delete; unique_resource& operator=(unique_resource const&) = delete; ~unique_resource() noexcept() { if (r_.has_value()) d_(r_.value()); } // ... Most of the code in the proposed unique_resource is implementing yet-another-optional. The invalid value detection does look a lot like what markable does. So i'm wondering to which extent the proposed unique_resource could not just be rewritten by: template<typename Resource, class Deleter, class Holder = optional<Resource>> class unique_resource { compressed_pair<Holder, Deleter> p_; ... }; and using unique_fd = unique_resource<int, fd_deleter, markable<fd_resource_traits> >; Am i missing something obvious here? Regards, Julien
On 12/6/23 14:58, Julien Blanc wrote:
Le 2023-12-05 16:54, Andrey Semashev via Boost a écrit :
On 12/5/23 17:34, Julien Blanc via Boost wrote:
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 think there's a misunderstanding here. My point was exactly about asserting is_nothrow_invocable for the scope_fail constructor argument, not for its destructor.
It doesn't matter where the assert is. I disagree with the requirement itself.
[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.
It does, and in a way that is very similar to unique_resource: https://github.com/akrzemi1/markable/blob/master/documentation/overview.adoc...
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.
A very basic implementation of unique_resource could ressemble this:
template<typename Resource, class Deleter> class unique_resource { std::optional<Resource> r_; Deleter d_; public: unique_resource(unique_resource const&) = delete; unique_resource& operator=(unique_resource const&) = delete; ~unique_resource() noexcept() { if (r_.has_value()) d_(r_.value()); } // ...
Most of the code in the proposed unique_resource is implementing yet-another-optional.
unique_resource doesn't work like optional, the resource object stays constructed for the whole lifetime of the owner unique_resource object. For example, the TS does not require the resource to be in allocated state to be able to call unique_resource::get().
The invalid value detection does look a lot like what markable does. So i'm wondering to which extent the proposed unique_resource could not just be rewritten by:
template<typename Resource, class Deleter, class Holder = optional<Resource>> class unique_resource { compressed_pair<Holder, Deleter> p_; ... };
and using unique_fd = unique_resource<int, fd_deleter, markable<fd_resource_traits> >;
Am i missing something obvious here?
A significant part of the unique_resource implementation is duplicated to support resource types with unallocated values. See the unique_resource_data class. That part could be changed to rely on `markable`, but that would not eliminate the need to specialize for `markable` instead of for resource traits. This is because unique_resource provides an interface that is different from `markable`, and there has to be some part of the implementation that provides that interface and knows how to interact with `markable`. A lot of implementation complexity is also because of the exception safety requirements. For example, unique_resource is supposed to call the deleter on the resource, if its construction fails with an exception. Note that both the resource and the deleter may fail to construct, which is the reason why resource_holder and deleter_holder exist in the current implementation. I don't see that part of the implementation going away with `markable` either. So in the end, what `markable` would eliminate is the need in a couple of type traits (has_custom_default and has_deallocated_state), which is a very minor part of the code. The rest would stay more-or-less the same. And let's not forget that `markable` itself has some level of complexity. But more importantly, there are design issues with this approach. What would `unique_resource<markable<T>>::get()` return? And what would be passed to the deleter? If it's `markable<T> const&` then that would be difficult to use in practice, as you would need to call `fd.get().value()` to access the actual file descriptor. And for deleters, it would make it impossible to specify raw functions like `close` as deleters. So it looks like `unique_resource` would have to unwrap `markable` internally, which adds another level of complexity. And it wouldn't work anyway because `markable` requires that the stored value is not in the marked state to be able to call `value()`. Same problem as with `optional`. So, with all things considered, it looks like `markable` is incompatible with `unique_resource` interface. And even if it was, it would only make thinks more complicated.
On 12/6/23 17:25, Andrey Semashev wrote:
On 12/6/23 14:58, Julien Blanc wrote:
Le 2023-12-05 16:54, Andrey Semashev via Boost a écrit :
On 12/5/23 17:34, Julien Blanc via Boost wrote:
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 think there's a misunderstanding here. My point was exactly about asserting is_nothrow_invocable for the scope_fail constructor argument, not for its destructor.
It doesn't matter where the assert is. I disagree with the requirement itself.
To clarify, I was also talking about the requirement of the action being is_nothrow_invocable. Scope guards' destructor noexcept markup is directly related to the action's operator() noexcept markup.
wt., 5 gru 2023 o 16:55 Andrey Semashev via Boost <boost@lists.boost.org> 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... 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
participants (3)
-
Andrey Semashev
-
Andrzej Krzemienski
-
Julien Blanc