Moving vector from class, return && or move into temporary?

Question

class EntityFactory
{
public:
    EntityFactory(tinyxml2::XMLElement * pEntitiesNode);
    ~EntityFactory();

    std::vector< std::unique_ptr<Entity> > && TakeEntities();

protected:
    std::vector< std::unique_ptr<Entity> > mEntities;
};

When I call TakeEntity I want to move the mEntities vector outside of this class.

Should I return a temporary variable that I moved into or return a &&?

std::vector< std::unique_ptr<Entity> > && EntityFactory::TakeEntities()
    {
    return std::move(mEntities);
    }

//or
std::vector< std::unique_ptr<Entity> > EntityFactory::TakeEntities()
    {
    return std::move(mEntities);
    }

The end result I want is that I can call this function from somewhere else and move the field vector into a variable near the call site.

Answer 1

The right way would presumably be to return by value and only do so if the instance is movable-from:

class Foo
{
    std::unique_ptr<Bar> bar_;

public:
    std::unique_ptr<Bar> get_bar() &&
    {
        return std::move(bar_);
    }
};

That way you can only move from the member if the entire instance is itself expiring. There's no need to return an rvalue reference in general, since that gains you very little, and comes at a big cost of not being generally safe. For example, suppose you have Foo f();, then consider:

auto && p = f().get_bar();

This is safe if you return by value, but would be a dangling reference if you returned by reference.

Answer 2

If you disable compiler copy elision, use the first one because the second one will generate an extra temporary variable which will slow down the performance.

If you did not disable the copy/move elision, they are most likely the same (performance).

Answer 3

You should not return a rvalue reference and might define a proper entity:

#include <iostream>
#include <vector>

struct Entity {
    int value;
    Entity() {
        static int n;
        value = ++n;
    }

    Entity(Entity&&) = default;
    Entity& operator = (Entity&&) = default;

    Entity(const Entity&) = delete;
    Entity& operator = (const Entity&) = delete;
};

class EntityFactory
{
    public:
    EntityFactory() {};

    void insert(Entity&& entity) {
        return mEntities.push_back(std::move(entity));
    }

    std::vector<Entity> Entities() {
        return std::move(mEntities);
    }

    protected:
    std::vector<Entity> mEntities;
};

int main() {
    EntityFactory factory;
    factory.insert(Entity());
    factory.insert(Entity());
    factory.insert(Entity());
    std::vector<Entity> entities = factory.Entities();
    for(const auto& e: entities) {
        std::cout << e.value;
    }
    std::cout << std::endl;
    if(factory.Entities().empty())
        std::cout << "Empty factory\n";
}

The target (the value getting the result of the function call) will be a copy elided value or a rvalue constructed value.