Reference<T> should have noexcept move constructor and move assignment operator

[szali]

I have a class which has a Napi::Reference member. This class of mine defines all 6 operations (ctor, dtor, copy ctor, move ctor, copy assignment, move assignment). I cannot declare the move operations noexcept since Napi::Reference has non-noexcept move operations, but since I support the copy operations (via own logic), std::vector cannot use my class efficiently.

The point is, Reference should have a noexcept move constructor and move assignment operator to facilitate usage with std containers, since it has only two handles and one bool member, which are trivial to assign, swap and zero out.

[szali]

Note that the current implementation of move assignment cannot be noexcept since it destroys the object first (with a potentially throwing Reset()), instead of using swaps (preferably with ADL) which would allow nothrow behaviour.

Current code:

template
inline Reference& Reference::operator =(Reference&& other) {
Reset();
_env = other._env;
_ref = other._ref;
_suppressDestruct = other._suppressDestruct;
other._env = nullptr;
other._ref = nullptr;
other._suppressDestruct = false;
return *this;
}

Should be:

template
inline Reference& Reference::operator =(Reference&& other) noexcept {
using std::swap;
swap(_env, other._env);
swap(_ref, other._ref);
swap(_suppressDestruct, other._suppressDestruct);
return *this;
}

The moved-from object will then take care of releasing resources when destroyed.

[gabrielschulhof]

@szali would you like to create PR implementing your suggestion?

[github-actions]

This issue is stale because it has been open many days with no activity. It will be closed soon unless the stale label is removed or a comment is made.

[legendecas]

I'd be interested in taking this up as it is related to exceptions.

[github-actions]

This issue is stale because it has been open many days with no activity. It will be closed soon unless the stale label is removed or a comment is made.

[KevinEady]

Hi @szali ,

We looked at this in today's Node-API meeting. The move assignment cannot be no-except because the reference in the to-be-assigned-into value needs to be deleted. As you noted, this operation can throw.

Unless we are misunderstanding something...?

[szali]

@KevinEady please see my comment above about implementing move semantics in a noexcept way with swap. This is how it is usually done. The deletion is then deferred until the moved-from value is destroyed.

The argument about the resource having to be deleted could be applied to e.g. std::vector, but it nevertheless has nothrow move assignment, see https://en.cppreference.com/w/cpp/container/vector/operator%3D. In fact, having to implement a move assignment operator implies (or should imply) that the class manages a resource, so if your argument were true then noexcept move would not exist.

[KevinEady]

@szali I think I see, for my understanding then, by "the deletion is then deferred until the moved-from value is destroyed", you mean:

• remove cleanup from this move
• handle the cleanup in ~Reference [already done]

correct?

[szali]

@KevinEady yes
e.g. see here, the "Move by swap" part: https://codereview.stackexchange.com/questions/182952/move-aware-noexcept-stack-in-c

[mhdawson]

@szali based on our discussion in the team meeting. The concern with your suggestion is:

• We are not doing a swap between the 2 objects, we are freeing the existing value then copying over new the new one. This results in a single object remaining, not 2 which have separate changed values.
• The free requires and Node-API call which is what precludes the noexcept
• If we swapped instead, we would have to do the free as a later step, and if that can cause an exception (which it can due to the Node-api call) then the overall operation is still not noexcept.

[github-actions]

This issue is stale because it has been open many days with no activity. It will be closed soon unless the stale label is removed or a comment is made.