Skip to content

Remove try/catch block on unique_resource move constructor#20

Open
windowsair wants to merge 1 commit into
nonstd-lite:masterfrom
windowsair:fix-clang-error
Open

Remove try/catch block on unique_resource move constructor#20
windowsair wants to merge 1 commit into
nonstd-lite:masterfrom
windowsair:fix-clang-error

Conversation

@windowsair

Copy link
Copy Markdown

The clang-21 says:

include/nonstd/scope.hpp:1007:40: error:
cannot refer to a non-static member from the handler of a constructor function try block
[-Werror,-Wexceptions]
1007 | other.get_deleter()( this->get() );
| ^
1 error generated.

Since the move constructor already provides an noexcept guarantee, no exceptions will be thrown here. The try-catch block is redundant.

And a further approach is to provide a throwable exception variant of the move constructor.

The clang-21 says:

include/nonstd/scope.hpp:1007:40: error:
      cannot refer to a non-static member from the handler of a constructor function try block
      [-Werror,-Wexceptions]
 1007 |             other.get_deleter()( this->get() );
      |                                        ^
1 error generated.

Since the move constructor already provides an noexcept guarantee,
no exceptions will be thrown here. The try-catch block is redundant.

And a further approach is to provide a throwable exception variant of
the move constructor.
@martinmoene

Copy link
Copy Markdown
Collaborator

There seems to be a bit more to it than providing the move constructor as:

 unique_resource( unique_resource && other )
        scope_noexcept_op(
            std11::is_nothrow_move_constructible<R1>::value && std11::is_nothrow_move_constructible<D>::value
        )
        : resource( conditional_move( std::move(other.resource), typename std11::bool_constant< std11::is_nothrow_move_assignable<R>::value >() ) )
        , deleter(  conditional_move( std::move(other.deleter ), typename std11::bool_constant< std11::is_nothrow_move_constructible<D>::value >() ) )
        , execute_on_reset( std14::exchange( other.execute_on_reset, false ) )
    {}

Proposal p0052 writes:

unique_resource(unique_resource&& rhs) noexcept (see below )

9 Effects: First, initialize resource as follows:
—(9.1) If is_nothrow_move_constructible_v<R1> is true, from std::move(rhs.resource);
—(9.2) otherwise, from rhs.resource.
[Note: If initialization of resource throws an exception, rhs is left owning the resource and will free it in due time. — end note ]
Then, initialize deleter as follows:
—(9.3) If is_nothrow_move_constructible_v<D> is true, from std::move(rhs.deleter);
—(9.4) otherwise, from rhs.deleter.
If initialization of deleter throws an exception and is_nothrow_move_constructible_v<R1> is true and rhs.execute_on_reset is true:
rhs.deleter(RESOURCE );
rhs.release();
Finally, execute_on_reset is initialized with exchange(rhs.execute_on_reset,false).

10 [Note: The explained mechanism ensures no leaking and no double release of resources. — end note ]

11 Remarks: The expression inside noexcept is equivalent to
is_nothrow_move_constructible_v<R1> && is_nothrow_move_constructible_v<D>.

@windowsair

Copy link
Copy Markdown
Author

If my understanding is correct, the two variants below still require implementation:

// from std::move(other.resource)
unique_resource( unique_resource && other )
        scope_noexcept_op(
            std11::is_nothrow_move_constructible<R1>::value && 
            !std11::is_nothrow_move_constructible<D>::value
        ) {}

// from other.resource
unique_resource( unique_resource && other )
        scope_noexcept_op(
            !std11::is_nothrow_move_constructible<D>::value
        ) {}

@martinmoene

Copy link
Copy Markdown
Collaborator

Thanks @windowsair , I'll look into it (don't hold your breath :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants