Re: [scope] review

From: Julien Blanc (julien.blanc_at_[hidden])
Date: 2023-12-06 11:58:21

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#defining-a-custom-marked-value-policy

> 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