Make Node move constructor and assignment operator noexcept (#809)

[TedLyngmo]

Move constructor:

 * m_isValid    (bool)                 exchange(rhs.m_isValid, true)
 * m_invalidKey (std::string)          std::move()
 * m_pMemory    (shared_memory_holder) std::move()
 * m_pNode      (node*)                exchange(rhs.m_pNode, nullptr)

This leaves the moved-from Node as if it was just default constructed.

Move assignment:

• A sanity test is performed to check if it's a valid move, and if not: *this is returned (with an added assert() for debugging).

• A temporary Node is move constructed (using the new move constructor), leaving the moved-from Node as if it was just default constructed.

• If this->m_pNode == nullptr, the same operation as AssignNode would do is done and *this is returned.

• if temporary.m_pNode == nullptr: m_pNode->set_null() swap(*this, temporary) return *this;

Otherwise the merge step that AssignNode would do if both m_pNodes are not nullptr is done, using a new member function, AssignNodeDetail().

Signed-off-by: Ted Lyngmo [email protected]

[TedLyngmo]

This one probably needs reading through once or twice more. I'll be testing it at work tomorrow. For some reason I'm not catching as many linting problems at home.

[TedLyngmo]

Making move assignment noexcept was trickier than I anticipated and I hope I haven't messed it up completely. :-)

[jpan127]

Hi, what's the status on this PR?

It seems like it is not only making the move special functions noexcept, but actually defining them for the first time. It would be great if this library supported moves.

[TedLyngmo]

Hi, what's the status on this PR?

Howdy and thanks for showing interest in this one!

It seems like it is not only making the move special functions noexcept, but actually defining them for the first time. It would be great if this library supported moves.

It's ongoing (even though it's not been updated for a some time). I'm on a rebase mission to prepare for this change actually, so I've put it on hold for now, but I will need to face it soon. I have a library with classes carrying a YAML::Node which makes moving my class' objects potentially throwing, which will not be accepted in the long run, so something needs to be done.

I think (@jbeder will have to pitch in) that the move constructor in this PR will never destroy any existing functionality, so that part of the PR could be separated and submitted on its own so that the complicated move assignment operator would be the only one left. Perhaps that'd be a way to take a step forward?

I've also been thinking about making a lot of test cases for things that this PR (or something like it) could break and deliver those long before this PR - but the thing is that don't have that deep knowledge of the inner workings of this lib that I wish I had, so the test cases I come up with is stuff that I sort of stumbe on while trying out new approaches and fix anyway. I'd be good if someone else wrote them. If you have ideas for tests etc., please do PR's to this branch in my fork and we can make an effort together.

[TedLyngmo]

Another PR would be one that makes a deliberate breaking change so that moving really means moving. There are one or two test cases (regarding Aliases) that refuses that - but I could make an outline for such a change. It'd break those test cases and perhaps programs making use of the functionality though.

As it is, it feels (for lack of better words) that any program using the copy magic when moving relies on side effects that should not be trusted in the first place, but I could be wrong. I'd like such a PR better because it'd make moving a YAML::Node comprehensible and more in sync with the rest of the move semantics we're used to.

[rr-mark]

This PR might resolve https://github.com/jbeder/yaml-cpp/issues/1209 and https://github.com/jbeder/yaml-cpp/issues/721. I think there are a number of assumptions made by std:: algorithms about how move constructors behave which are not met by YAML::Nodes at present.

I briefly looked into adding default move constructors and move assignment to YAML::Node, but this breaks a couple of unit tests (NodeTest.SimpleAlias and NodeTest.TempMapVariableAlias), and more generally leads to a difference in behaviour between copy assignment and move assignment, which probably has many downstream consequences.