kpeeters / tree.hh

An STL-like C++ header-only tree library
GNU General Public License v3.0
132 stars 32 forks source link

Inserting data into the tree using an empty `sibling_iterator` and `std::move` results in undesired behavior #24

Closed danieto98 closed 10 months ago

danieto98 commented 10 months ago

Problem When calling insert with an empty sibling_iterator as the first parameter, it is expected that a new node will be inserted below that iterator's parent. This is indeed the case if the insert method is called using such a sibling_iterator and data passed by constant reference. However, when calling the method using std::move on the data instead, the new node is inserted at the end of the tree.

The following code snippet illustrates this:

#include <vector>

#include "tree.hh"
#include "tree_util.hh"

int main(int argc, char *argv[])
{
    std::vector<std::vector<int>> idxsVec = {{0, 0, 0}, {0, 0, 1}, {0, 1, 2}, {1, 1, 2}};

    tree<int> tr;
    for(auto &idxs : idxsVec)
    {
        tree<int>::sibling_iterator it = tr.begin();
        tree<int>::sibling_iterator endIt = tr.end();

        for (auto &idx : idxs)
        {
            auto currIt = std::find_if(it, endIt, [idx](int foundIdx){
                return idx == foundIdx;
            });
            if(currIt == endIt)
            {
                // Option 1
                currIt = tr.insert(endIt, idx);

                // Option 2
                // currIt = tr.insert(endIt, std::move(idx));
            }

            it = tr.begin(currIt);
            endIt = tr.end(currIt);
        }
    }

    kptree::print_tree_bracketed(tr);

    return 0;
}

Using Option 1 you get this output (expected):

0(0(0, 1), 1(2))
1(1(2))

Using Option 2, this happens:

0
0
0
0
1
1
2
1
2

Possible solution It seems the problem comes from the fact that there is a specialization of the insert method accepting sibling_iterator objects and data passed by constant reference, but there isn't one accepting moved data. This forces the code in Option 2 above to default to the generic version of the insert method that does accept moved data. Solving it seems as simple as creating such a specialization.

kpeeters commented 10 months ago

Good catch. I have pushed a fix, please let me know if that solves the problem.

danieto98 commented 10 months ago

Yes, it's solved! Thanks!