Move node to first position

[a-w50]

Hi,

is it possible to use the node move operation in order to move a node along it's siblings to the first place? Right now, there is only an after option. Using a NodeRef {tree, c4::yml::NONE } as after, doesn't seem to work, as it is not a sibling.

Thanks for this library!

[biojppm]

Did you try this method? It should accept the empty ConstNodeRef{tree, NONE}.

On the other hand, the method that is given a parent will do consistency checks.

Maybe you're using this second method where you actually wanted the first?

[a-w50]

Thank you for your quick reply,

if I use this method, it will call

rapidyaml/src/c4/yml/tree.cpp

Line 985 in 74c8031

void Tree::move(size_t node, size_t after)

and there are the has_sibling checks, which fail for the NodeRef{tree, NONE} node, because it isn't part of the tree. Or are there other ways to create/get this pseudo NodeRef?

[biojppm]

Ah, got it. This is a bug indeed.

But maybe you can work around it for now. Looking at this line:

rapidyaml/src/c4/yml/tree.cpp

Line 989 in 74c8031

_RYML_CB_ASSERT(m_callbacks, has_sibling(node, after) && has_sibling(after, node));

It seems that the assert there is not exactly right, and should accomodate NONE. It you change it to

_RYML_CB_ASSERT(m_callbacks, (after == NONE) || (has_sibling(node, after) && has_sibling(after, node)));

what happens? Let me know if it works. Or worst case, you can just call those functions on your own:

void mymove(Tree &tree, size_t node, size_t after)
{
    tree._rem_hierarchy(node);
    tree._set_hierarchy(node, tree.parent(node), after);
}

(I will fix it on my own anyway and add proper unit tests, but for now it's just hard to get the time to look at it; the suggestions above are just to get you going quicker.)

[biojppm]

By the way, _set_hierarchy() accomodates this case:

rapidyaml/src/c4/yml/tree.cpp

Line 631 in 74c8031

_RYML_CB_ASSERT(m_callbacks, iprev_sibling == NONE || (iprev_sibling >= 0 && iprev_sibling < m_cap));

[a-w50]

Right, I think there needs to be a check if it is NONE or not. I can't call _xxx_hierarchy because they are private. Btw, I think the has_sibling routine is implemented quite expensive (full linear search), wouldn't it be enough to check if both have the same parent (with special treatment of the NONE of course).

Btw. right now I'm working around this issue by

auto first_elem = parent.first_child();
elem.move(first_elem);
first_elem.move(elem);

[biojppm]

@a-w50 I prepared a PR, and improved the tests to address this scenario as well. The tests pass on my PC, but let's wait for the whole suite to finish in the CI.

FWIW, you can move the subject node to the first position within its parent by simply doing

subject.move({});

And if the parent is a different one, either on the same or on a different tree:

subject.move(new_parent, {});

[a-w50]

👍
Concerning the has_sibling function, wouldn't it be easier to check if both have the same parent, instead of searching each other?