erikerlandson / st_tree

A fast and flexible c++ template class for tree data structures
https://github.com/erikerlandson/st_tree/wiki
Apache License 2.0
95 stars 20 forks source link

Strange behavior of t1.graft(t2) #24

Closed jwdevel closed 4 years ago

jwdevel commented 4 years ago

I just started using this code, so I was surprised by the behavior of:

t1.graft(t2)

In particular, it seems to:

The behavior I wanted can actually be accomplished by:

t1.graft(t2.root()).

which combines the two trees.

Perhaps the first code I wrote should not compile? Or perhaps it should do what the second code does? I don't see how the current behavior of t1.graft(t2) is desirable at all, but maybe I'm not thinking of the right situation.

erikerlandson commented 4 years ago

@jwdevel thanks! I'll take a look at it and figure out what the policy should be.

erikerlandson commented 4 years ago

Assuming I'm reading my own doc and tests right, I'd expect t1.graft(t2) and t1.graft(t2.root()) to behave the same.

Note that in either case, t2 is left empty, so if you are doing it twice with the same t2, the second time it will start empty and not affect t1.

If you are seeing it misbehave, have a reproducer, feel free to post the reproducing code here and I'll play with it!

jwdevel commented 4 years ago

I played around with it a little more — I had it wrong before; t1 is not untouched, but it is replaced by t2. This is still unexpected to me since I expect "graft" to combine the two trees, not replace one with the other.

Here's code I've been testing with:

    tree<string> t1;
    t1.insert("foo");

    tree<string> t2;
    t2.insert("bar");

    // These both result in t1's "foo" being discarded.
    // At the end, t1 only contains "bar" (and t2 is empty).
    //t1.graft(t2);
    //t1.graft(t2.root());

    // These both create a tree with "foo" at ply0 and "bar" at ply1
    t1.root().graft(t2);
    //t1.root().graft(t2.root());

    cout << "t1 size after graft:  " << t1.size() << endl;
    cout << "t1 depth after graft: " << t1.depth() << endl;
    cout << "t2 size after graft:  " << t2.size() << endl;
    cout << "t2 depth after graft: " << t2.depth() << endl;

    for (auto it=t1.begin(); it!=t1.end(); ++it) {
        auto i = *it;
        cout << "ITEM: iply=" << i.ply() << " itply=" << it->ply() << " value=" << i.data() << endl;
    }

So, I guess the real issue is that t.graft(x) is different from t.root().graft(x) ?

erikerlandson commented 4 years ago

So, I guess the real issue is that t.graft(x) is different from t.root().graft(x)

Yes, it has to do with the distinction between tree<> and node<>. It's not a "code" bug, but it is arguably a documentation bug, since the behaviors aren't really detailed anywhere.

erikerlandson commented 4 years ago

@jwdevel is this working for you (excepting my documentation), or are you still having troubles using the library?

jwdevel commented 4 years ago

Yes, it has been working fine; I just had the initial confusion.

Feel free to close this. It still seems odd to me that tree.graft means "replace" rather than "combine", but oh well (:

erikerlandson commented 4 years ago

OK - I added some clarification to the docs on graft for tree<>. I'll close this.