Why does std::vector call the destructor while leaving a different scope?

Question

I have some code like this

std::vector<cuarl_path::Path> RoverPlanner::get_paths(const char *filename) const
{
    pugi::xml_document doc;
    doc.load_file(filename);
    pugi::xml_node root = doc.document_element();

    std::vector<cuarl_path::Path> paths;
    for (auto path_node : root.children()) {
        std::vector<cuarl_path::Segment*> segments;
        for (auto segment_node : path_node.children())
        {
            //do stuff to populate the `segments` vector
        }

        cuarl_path::Path* path = new cuarl_path::Path(segments);
        paths.push_back(*path); //Path destructor called here
    }

    return paths;
}

Here's the call stack

The issue seems that the std::vector "paths" calls its destructor, but I don't understand why. The scope has not ended.

Answer 1

paths.push_back(*path);

this does a few things.

1. It constructs a Path copy from the heap-allocated *path object.

2. It resizes the paths vector. This can sometimes involve just increasing an integer, but at other times it involves moving every existing Path object and destroying the old ones.

In the first point, you have a leak. new allocates objects on the free store, you are always responsible for ensuring they are cleaned up. Copying the object off the free store into a vector does not clean up the object on the free store.

In the second point, vector actually holds actual objects, not references to them. So when you resize the buffer (which push_back can do), you have to move the values of the objects around, then clean up the ones you are discarding.

That cleanup is the destructor you are doing.

You appear to be a C# or Java programmer. In both of those languages, actual values of objects are really hard to create -- they want to hold onto garbage collected references to objects. An array of objects is actually an array of references to objects in those languages. In C++, a vector of objects is a vector actually containing the objects in question.

Your use of new is also a tip. There is no need for new there, nor is there need for a pointer:

std::vector<cuarl_path::Path> RoverPlanner::get_paths(const char *filename) const
{
    pugi::xml_document doc;
    doc.load_file(filename);
    pugi::xml_node root = doc.document_element();

    std::vector<cuarl_path::Path> paths;
    for (auto&& path_node : root.children()) {
        std::vector<cuarl_path::Segment*> segments;
        for (auto segment_node : path_node.children())
        {
            //do stuff to populate the `segments` vector
        }

        cuarl_path::Path path = cuarl_path::Path(segments);
        paths.push_back(std::move(path)); //Path destructor called here
    }

    return paths;
}

you'll still get path destructors called on the line (in fact, you'll get an extra one), but your code won't leak. Assuming your move constructor is correct (and in fact moves the state of the path properly), everything should work.

Now a more efficient version looks like:

std::vector<cuarl_path::Path> RoverPlanner::get_paths(const char *filename) const
{
    pugi::xml_document doc;
    doc.load_file(filename);
    pugi::xml_node root = doc.document_element();

    std::vector<cuarl_path::Path> paths;
    auto&& children = root.children();
    paths.reserve(children.size());
    for (auto path_node : children) {
        std::vector<cuarl_path::Segment*> segments;
        for (auto segment_node : path_node.children())
        {
            //do stuff to populate the `segments` vector
        }

        paths.emplace_back(std::move(segments));
    }

    return paths;
}

which gets rid of all of the temporary variables you are messing with, and moves resources when they are of no more use.

Assuming efficient move constructors, the big gains here are that we pre-researve the paths vector (saving lg(n) memory allocations) and we move the segments vector into the constructor of a path (if written right, that could avoid a needless copy of the buffer of segment pointers).

This version also has no destructors called on the line in question, but I don't consider that to be particularly important; the cost of a destructor of an empty path should be nearly free, and even plausible to optimize away.

I also possibly avoided a copy of the path_node object, which could be worth avoiding, depending on how it is written.