Logo Questions Linux Laravel Mysql Ubuntu Git Menu
 

Putting non-copyable objects into std-containers

Is this class design the standard C++0x way to prevent copy and assign, to protect client code against accidental double-deletion of data?

struct DataHolder {
  int *data;   // dangerous resource
  DataHolder(const char* fn); // load from file or so
  DataHolder(const char* fn, size_t len); // *from answers: added*
  ~DataHolder() { delete[] data; }

  // prevent copy, to prevent double-deletion
  DataHolder(const DataHolder&) = delete;
  DataHolder& operator=(const DataHolder&) = delete;

  // enable stealing
  DataHolder(DataHolder &&other) {
    data=other.data; other.data=nullptr;
  }
  DataHolder& operator=(DataHolder &&other) {
    if(&other!=this) { data = other.data; other.data=nullptr};
    return *this;
  }
};

You notice, that I defined the new move and move-assign methods here. Did I implement them correctly?

Is there any way I can -- with the move and move-assign definitions -- to put DataHolder in a standard container? like a vector? How would do I do that?

I wonder, some options come into mind:

// init-list. do they copy? or do they move?
// *from answers: compile-error, init-list is const, can nor move from there*
vector<DataHolder> abc { DataHolder("a"), DataHolder("b"), DataHolder("c") };

// pushing temp-objects.
vector<DataHolder> xyz;
xyz.push_back( DataHolder("x") );
// *from answers: emplace uses perfect argument forwarding*
xyz.emplace_back( "z", 1 );

// pushing a regular object, probably copies, right?
DataHolder y("y");
xyz.push_back( y ); // *from anwers: this copies, thus compile error.*

// pushing a regular object, explicit stealing?
xyz.push_back( move(y) );

// or is this what emplace is for?
xyz.emplace_back( y ); // *from answers: works, but nonsense here*

The emplace_back idea is just a guess, here.

Edit: I worked the answers into the example code, for readers convenience.

like image 792
towi Avatar asked Apr 16 '11 15:04

towi


2 Answers

Your example code looks mostly correct.

  1. const DataHolder &&other (in two places).

  2. if(&other!=this) in your move assignment operator looks unnecessary but harmless.

  3. The initializer list vector constructor won't work. This will try to copy your DataHolder and you should get a compile time error.

  4. The push_back and emplace_back calls with rvalue arguments will work. Those with lvalue arguments (using y) will give you compile time errors.

There really is no difference between push_back and emplace_back in the way that you have used them. emplace_back is for when you don't want to construct a DataHolder outside of the vector, but instead pass the arguments to construct one only inside the vector. E.g.:

// Imagine this new constructor:
DataHolder(const char* fn, size_t len);

xyz.emplace_back( "data", 4 );  // ok
xyz.push_back("data", 4 );  // compile time error

Update:

I just noticed your move assignment operator has a memory leak in it.

DataHolder& operator=(DataHolder &&other)
{
   if(&other!=this)
   {
      delete[] data;  // insert this
      data = other.data;
      other.data=nullptr;
   }
   return *this;
}
like image 174
Howard Hinnant Avatar answered Sep 28 '22 05:09

Howard Hinnant


Temporary objects that hold no name, eg. DataHolder("a"), are moved when available. The standard container in C++0x will always move when possible, that's what also allows std::unique_ptr to be put in a standard container.
Besides that, you implemented your move operations wrong:

  // enable stealing
  DataHolder(const DataHolder &&other) {
    data=other.data; other.data=nullptr;
  }
  DataHolder& operator=(const DataHolder&&other) {
    if(&other!=this) { data = other.data; other.data=nullptr};
    return *this;
  }

How do you move from a constant object? You can't just change the other's data, as that other is constant. Change that to simple DataHolder&&.

like image 29
Xeo Avatar answered Sep 28 '22 05:09

Xeo