CODEWITHSUNDEEP
c++c++11
ghord
ghord

Reputation: 13835

Passing std::vector to constructor and move semantics

Durning the weekend I'm trying to refresh my c++ skills and learn some c++11, I've stumbled onto the following problem: I cannot force my container class to properly use move constructor:

I have a builder class, which is defined as follows:

class builder
{
   ...
   container build() const
   {
     std::vector<items> items;

     //... fill up the vector

     return container(items); //should move the vector right? wrong!
     //return container(std::move(items)); also doesn't work
   }
}

And classes item and container, defined as follows:

class container
{
public:

    container(std:vector<item> items)
      : items_(items) // always invokes copy constructor on vector, never move
    { }

    container(container&& rhs)
    {
       ...
    }

    ...

private:
    std::vector<item> items_;

}

class item
{
public:
    //move .ctor
    item(item && rhs);
    item& operator=(item && rhs);

    //copy .ctor
    item(const item& rhs); //this gets called instead of move .ctor
    item& operator=(const item& rhs);

    ...
}

Now my code simply uses 

builder my_builder;
...
auto result = my_builder.build();

which causes every item to be first constructed and then copied...

How should I write following classess to not copy items? Should I just go back to using standard pointers?

Upvotes: 13

Views: 20789

Answers (2)

CodeAngry
CodeAngry

Reputation: 12995

Wrote this template move-enabled class for you. Study it and you'll get it.

/// <summary>Container.</summary>
class Container {
private:
    // Here be data!
    std::vector<unsigned char>  _Bytes;

public:
    /// <summary>Default constructor.</summary>
    Container(){
    }

    /// <summary>Copy constructor.</summary>
    Container(const Container& Copy){
        *this = Copy;
    }

    /// <summary>Copy assignment</summary>
    Container& operator = (const Container& Copy){
        // Avoid self assignment
        if(&Copy == this){
            return *this;
        }
        // Get copying
        _Bytes = Copy._Bytes; // Copies _Bytes
        return *this;
    }

    /// <summary>Move constructor</summary>
    Container(Container&& Move){
        // You must do this to pass to move assignment
        *this = std::move(Move); // <- Important
    }

    /// <summary>Move assignment</summary>
    Container& operator = (Container&& Move){
        // Avoid self assignment
        if(&Move == this){
            return *this;
        }
        // Get moving
        std::swap(_Bytes, Move._Bytes); // Moves _Bytes
        return *this;
    }
}; // class Container

I'm always against of using value arguments like this:

function(std:vector<item2> items)

I always use either:

function(const std:vector<item2>& items)
function(std:vector<item2>& items)
function(std:vector<item2>&& items)

especially for larger data containers, and seldom:

function(std:vector<item2> items)

for smaller data, never vectors.

This way, you're in control of what happens and that's why we do C++, to control everything.

  • If you need a writable copy, just copy the const reference yourself in a variable.
  • If you need it read-only, const reference prevents a new copy.
  • If you want to edit original, just use reference.
  • And use value arguments for small data when you feel lazy.

Obviously, it all depends on what you're doing.

I'm a self taught C++ developer. Far from an expert, especially in the C++ slang... but learning :)

Upvotes: 6

GManNickG
GManNickG

Reputation: 504313

Your code should be changed to this:

container(std:vector<item2> items) // may copy OR move
: items_(std::move(items)) // always moves
{}

In general: if you want your own copy of something then make that copy on that parameter list and move it to where it needs to be. Let the caller be the one that decides if they are going to copy or move the existing data. (In other words, you were halfway there. Now just move your data.)

Also: return container(std::move(items));. I didn't mention this before because I mistakenly thought all local variables were automatically moved in a return statement, but only the returned value is. (So this should actually work: return items;, because container's constructor is not explicit.)

Upvotes: 27

Related Questions