Context: I have a singly-chained list of Nodes, and calling PruneRecursively on its root should prune certain nodes from it, preserving the rest of the chain. Each node knows privately whether it should stay or go.

The implementation I came up with looks as follows:

unique_ptr<Node> Node::PruneRecursively(unique_ptr<Node> self)
{
    if (m_ChildNode)
        m_ChildNode = m_ChildNode->PruneRecursively(std::move(m_ChildNode));
    
    if (ShouldRmThis())
        return std::move(m_ChildNode);
    else
        return self;
}

if (m_RootNode)
    m_RootNode = m_RootNode->PruneRecursively(std::move(m_RootNode));

Look at the very last line. It looks suspect to me, because m_RootNode is move-d before a function is called on it. It works, but I think could be bad if the method reset()-ed the pointer, basically delete-ing this, before accessing a member.

Is it fine (if so, why), or is it a catastrophe waiting to happen?

EDIT: Node::PruneRecursively would look better if it was static, but I need virtual dispatching (Node is polymorphic).

CodePudding user response：

Since C 17 it is guaranteed that the postfix expression in a function call is evaluated before any of the expressions in the argument list. That means operator-> on m_RootNode is guaranteed to be evaluated before the constructor of self is called. Therefore there is technically no problem.

Before C 17 there was no guarantee for this order and the constructor for self could be called before evaluation of the operator->. They were indeterminately sequenced. That would automatically cause undefined behavior because there would be a path in which operator-> is guaranteed to be called on an empty unique_ptr.

I would however advice against relying on these new C 17 guarantees. It doesn't cost you anything really to write out e.g. a reference variable to *m_RootNode first and then call the function on it.

Also, with regards to the overall design, not just the evaluation of that expression, see the comments under the question.