API: consider single parameter NodeRef::duplicate

[dancingbug]

NodeRef::duplicate: What's the rationale for having separate parameters parent and after?

Background: When I wanted to clone the descendants of a NodeRef into a new tree, intuition suggested I do the following:

NodeRef nd = ...
...
Tree tree{};
tree.rootref() |= MAP;
nd.duplicate(tree.rootref(), NodeRef{});

result: check failed: parent.m_tree == after.m_tree

Ok, how about...

NodeRef nd = ...
...
Tree tree{};
tree.rootref() |= MAP;
nd.duplicate(tree.rootref(), tree.ref(NONE));

result: check failed: (id != NONE && id >= 0 && id < m_size)

I used Tree::duplicate directly instead.

Why not just get the tree and parent from a single NodeRef parameter? proposed replacements:

• NodeRef::duplicate_after(NodeRef const after) (duplicates after given node) + NodeRef::duplicate_under(NodeRef const parent) (duplicates as first child of given node)

similarly for NodeRef::move etc.

[biojppm]

Might be related to #324, with eager assertions making it impossible.

[biojppm]

Indeed there was a problematic assertion preventing proper use of duplicate(); it has been fixed in the commit linked above.

As for the API suggestion, it is a good point. Passing a parent makes sense in the Tree API, but not as much in the NodeRef API. I will take some time to think about this, as I want the two APIs to be as similar as possible.

[dancingbug]

As for the API suggestion, it is a good point. Passing a parent makes sense in the Tree API, but not as much in the NodeRef API. I will take some time to think about this, as I want the two APIs to be as similar as possible.

Even within Tree , _set_hierarchy is already doing neighbor lookups

my 2c: might as well use parent(iprev_sibling)

further, here's my 'armchair proposal' for a more general API:

enum InsertionPoint {
  FIRST_CHILD,
  LAST_CHILD,
  PREV_SIBLING,
  NEXT_SIBLING,
  REVERSE_FIRST_CHILD,
  REVERSE_LAST_CHILD,
  REVERSE_PREV_SIBLING,
  REVERSE_NEXT_SIBLING
};
Tree::duplicate(size_t dst_node, Tree* src, size_t src_node, InsertionPoint p) //param ordering like memset
NodeRef::duplicate (NodeRef dst_node,InsertionPoint p)

The REVERSE_ variants would behave like this

Tree dst        Tree src
    a               w
   / \             / \
  b   c           x   v
                 / \
                y   z
                   / \
                  k   m

Tree dst after
dst.duplicate(b,&src,z,REVERSE_PREV_SIBLING)
                    a
                   / \
                  x   c
                 /|\
                y b z
                   / \
                  k   m

(node,InsertionPoint) (or, at least with the non-REVERSE_ variants) can be seen as a reification of "subtree with a hole", a more general version would involve an entire relative path instead of InsertionPoint. The REVERSE_ part could be split out into an enum CopyDirection {FORWARD,REVERSE} and then you pass say FIRST_CHILD | REVERSE to Tree::duplicate

(PS, Thanks for your nice library!)