13

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?

ghord
  • 13,260
  • 6
  • 44
  • 69

2 Answers2

27

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.)

GManNickG
  • 494,350
  • 52
  • 494
  • 543
  • 1
    The exception to this general rule is that you should pass by rvalue reference (rather than by value) if you are passing very large objects which are expensive to construct even when moved, or when you are writing generic code which is meant to be efficient regardless of the types of the objects that are passed. – Mankarse Apr 01 '12 at 10:57
  • What about the case when someone invokes this constructor with a vector that will be used later? Wouldn't that move contents of vector to the one in container and by doing so remove them from original vector? – ghord Apr 01 '12 at 11:00
  • @Mankarse: I don't see that as an exception to the rule because the rule already starts with "if you want your own copy...". If you don't need a copy, don't make one. If you do need one, expensive or not it's going to happen. – GManNickG Apr 01 '12 at 11:01
  • @Gregory: I don't follow. The data held in `items` is not accessible from anywhere but this constructor. You are free to move it. – GManNickG Apr 01 '12 at 11:02
  • 2
    The return statement in the function `build` should be changed to `return container(std::move(items));` otherwise there is an unnecessary copy of items. – mark Apr 01 '12 at 11:03
  • @mark: It is my understanding that in a return statement all local variables are automatically xvalues (expiring values) and will be moved, but I'm unsure if that only applies to the returned object itself. So OP should go ahead and put that in there until I'm more confident it shouldn't have to be. :) – GManNickG Apr 01 '12 at 11:10
  • @mark You are right. std::move in return statement is required, otherwise items are still copied. – ghord Apr 01 '12 at 11:15
  • @Gregory: Which compiler, by the way? If GCC, then I'm almost certainly wrong that all objects are moved. – GManNickG Apr 01 '12 at 11:19
  • @GManNickG: The point is that passing by rvalue reference avoids the need for _two_ copies/moves (one in the initialisation of the parameter (`items`) and one in the initialisation of the object that eventually gets constructed (`items_`)). – Mankarse Apr 01 '12 at 11:25
  • @Mankarse: I still don't follow. My code has minimal possible copies and moves to initialize `items_`, where do you think you save by taking the argument by rvalue reference? This is already done in the move-constructor for the parameter. – GManNickG Apr 01 '12 at 11:33
  • @GManNickG: It saves the (potentially expensive in the case of a very large object) move construction of `items`. – Mankarse Apr 01 '12 at 11:36
  • @Mankarse: Move construction is (in any code I would ever care to talk about) suppose to be the cheapest possible method of construction behind default construction. I understand what you're saying, but if your move constructor is that slow, the issue isn't moving, it's your move constructor. (Not to mention a compiler ought to inline the constructor code and not actually make two moves.) – GManNickG Apr 01 '12 at 11:38
  • Compiler is Visual Studio 11 beta – ghord Apr 01 '12 at 11:39
  • @Gregory: Ah. I'm a little reserved to change my understanding of the C++ language based off MSVC compilers, but I think upon further reading I can safely say I was wrong anyway, and that you should definitely include the `move`. – GManNickG Apr 01 '12 at 11:41
  • @GManNickG: Fair enough. As for objects automatically becoming rvalues in return statements, this only happens when that object is the _only_ thing in the return expression (see `[class.copy]/32`, or the [question](http://stackoverflow.com/questions/9963974/are-returned-locals-automatically-xvalues) about it). Basically - it only applies when copy elision would apply. – Mankarse Apr 01 '12 at 12:03
  • @GManNickG: Your original code is correct, there is no need to use `std::move` in the return statement. – mark Apr 01 '12 at 12:19
  • @mark: Now we've switched sides. :) If it's just `return items;` you're right, but what he has won't move `items`. The only thing implicitly moved is the value being returned. – GManNickG Apr 01 '12 at 20:38
  • @GManNickG oops when I added the second comment I missed the original call to the constructor of container! Ignore my second comment - your anawer is correct! – mark Apr 01 '12 at 21:24
6

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 :)

CodeAngry
  • 12,760
  • 3
  • 50
  • 57
  • 1
    [Efficient Argument Passing in C++11](http://www.codesynthesis.com/~boris/blog/2012/06/26/efficient-argument-passing-cxx11-part2/) ... good explanation. – CodeAngry Oct 16 '12 at 20:45