Putting non-copyable objects into std-containers

Question

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.

const RValue references? – Benjamin Lindley Apr 16 '11 at 15:29 — Benjamin Lindley, Apr 16 '11 at 15:29

Howard Hinnant · Accepted Answer · 2011-04-17T03:40:10.060

6

Your example code looks mostly correct.

~~const~~ DataHolder &&other (in two places).
if(&other!=this) in your move assignment operator looks unnecessary but harmless.
The initializer list vector constructor won't work. This will try to copy your DataHolder and you should get a compile time error.
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;
}

edited Apr 17 '11 at 03:40

answered Apr 16 '11 at 15:41

Howard Hinnant

206,506
52
449
577

1

I don't think preventing move-to-self is useless. After all, `a = std::move(a)` is valid and should be a no-op. – ltjax Apr 16 '11 at 15:57
(1) copy-paste-error. i edited that out. (2) really? isn't that the usual pattern for copy? temp-object can not be `this`? hmmm. (3) right, the init-list members behave `const`. ok. (4) Ah, thats "emplace"... of course. Now I understand. (*) Beautiful all those new things working together: rvalue-refs, perfect-forwarding, template-varargs. Great! – towi Apr 16 '11 at 17:00
3

@towi - The usual pattern for copy is the "copy-and-swap" idiom: `T& operator =(T rhs /* pass by value! */) { this->swap(rhs); return *this; }` This has three benefits: 1) delegates to copy ctor so no need to duplicate code; 2) is exception-safe iff swap does not throw (and it shouldn't!); and 3) self-assignment works without an explicit check. `this->swap(rhs)` (with `T&& rhs`) is also often a nice way to implement the move assignment operator. – JohannesD Apr 16 '11 at 17:58
@JohannesD waitwaitwait... Ok, the pass-by-value I can understand, thats where the delegation to copy-ctor happens. but what's with `T::swap(T&)`? I have to implement that, and that would be duplicate code. Ok, often we implement it anyway. Thats what you mean, right? Why is explicit self-check not needed? Because its done in `swap`? Hmm... because `rhs` is a copy. But that would make two copies, One implicit in the call-by-value, one explicit piece-by-piece in `swap(T&)`. Ok, thats cheap, I see! You suggest two `swap` impls, right? `swap(T&)` and `swap(T&&)`? Thx a billion... – towi Apr 16 '11 at 19:41
@JohannesD I just want to point out, that in this class I want to discourage the client code from using copy at all. Copy here is expensive, and `data` could also be a non-copyable resource -- a file, a lock. I don't like providing a copy-ctor for data-holding classes, you never know how often you copy, then. – towi Apr 16 '11 at 19:49
@towi - Yes, disallowing copy is perfectly okay, I was just talking in a more general sense. Actually, given that we're talking about C++11, implementing one's own `swap` (and specializing `std::swap`) is much less important than in C++03, because `std::swap` now moves things around instead of copying. This means, though, that you can't use `swap` to implement move assignment without getting infinite recursion... :P A self-assignment check isn't needed because the code works correctly without it - perhaps non-optimally but remember that self-assignment is rare and may be optimized out anyway. – JohannesD Apr 16 '11 at 20:16
@towi - The pass-by-value trick in the copy-and-swap idiom makes it possible for the compiler to elide the copying of `rhs` in cases `rhs` is a temporary. Even though we have move semantics now, eliminating a move or copy altogether is still faster than doing either of those :) – JohannesD Apr 16 '11 at 20:43

score 1 · Answer 2 · answered Apr 16 '11 at 15:31

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

ups, copy-paste-error with the `const&&`ies. I removed the consts so readers will not learn this faulty code. — towi, Apr 16 '11 at 16:55

Putting non-copyable objects into std-containers

2 Answers2