Re: [scope] review

From: Andrey Semashev (andrey.semashev_at_[hidden])
Date: 2023-12-06 14:25:38

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

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.