Can `*this` be `move()`d?

Question

I would like to define a class for marshalling data; when marshalling is finished, I would like to move the marshalled data out from within it, which will probably invalidate the marshalling object.

I believe this is possible with the static function extractData below:

class Marshaller
{
  public:
    static DataType extractData(Marshaller&& marshaller)
    {
      return std::move(marshaller.data);
    }
  private:
    DataType data;
}

This is a bit inconvenient to call, though:

Marshaller marshaller;
// ... do some marshalling...
DataType marshalled_data{Marshaller::extractData(std::move(marshaller))};

So can I wrap it with a member function?

DataType Marshaller::toDataType()
{
  return Marshaller::extractData(std::move(*this));
}

This would, of course, be called using:

DataType marshalled_data{marshaller.toDataType()};

...which, to me, looks much nicer. But that std::move(*this) thing looks awfully suspicious. In the context of the call to toDataType(), marshaller can't be used again, but I don't think the compiler can know that: the body of the function could be outside the caller's compilation unit, so there's nothing to indicate that marshaller has had move() applied to it.

Is this undefined behavior? Is it perfectly fine? Or somewhere in between? Is there a nicer way to accomplish the same goal, preferably without using a macro or requiring the caller to explicitly move marshaller?

EDIT: With both G++ and Clang++, I found that not only could I compile the above use case, but I could actually continue to make modifications to the underlying data via the marshaller, then re-extract the modified data using the toDataType function. I also found that the already-extracted data in marshalled_data continued to be changed by marshaller, which indicates that the marshalled_data is shared between the marshaller and the calling context, so I suspect that there is either a memory-leak or undefined behavior (from double-deletion) here.

EDIT 2: If I put a print statement in DataType's destructor, it appears twice when the caller leaves scope. If I include a data member in DataType that has an array in it, with a corresponding new[] and delete[], I get a glibc "double free or corruption" error. So I'm not sure how this could be safe, even though several answers have said that it's technically allowed. A complete answer should explain what is required to use this technique correctly with a non-trivial DataType class.

EDIT 3: This is enough of a rabbit-hole/can-of-worms that I've opened up another question to address my remaining concerns.

Answer 1

According to the standard the move-from object will still be valid although its state is not guaranteed, so it seems that moving from *this would be perfectly valid. Whether it's confusing to users of your code is another question entirely.

All that said it sounds like your real intention is to link the destruction of the marshallar with the extraction of the data. Did you consider doing all the marshalling within a single expression and letting a temporary take care of things for you?

class Marshaller
{
  public:
    Marshaller& operator()(input_data data) { marshall(data); return *this; }
    DataType operator()() { return std::move(data_); }
  private:
    DataType data_;
}

DataType my_result = Marshaller()(data1)(data2)(data3)();

Answer 2

I would avoid moving from *this, but if you do it, at least you should add rvalue ref-qualifier to the function:

DataType Marshaller::toDataType() &&
{
    return Marshaller::extractData(std::move(*this));
}

This way, the user will have to call it like this:

// explicit move, so the user is aware that the marshaller is no longer usable
Marshaller marshaller;
DataType marshalled_data{std::move(marshaller).toDataType()};

// or it can be called for a temporary marshaller returned from some function
Marshaller getMarshaller() {...}
DataType marshalled_data{getMarshaller().toDataType()};

Answer 3

There is nothing inherently unsafe about calling move(*this). The move is essentially just a hint to a function being called that it may steal the internals of the object. In the type system, this promise is expressed through && references.

This is not related to destruction in any way. The move does not perform any type of destruction - as mentioned, it just enables us to call functions taking && parameters. The function receiving the moved object (extractData in this case) also does not do any destruction. In fact, it needs to leave the object in a "valid but unspecified state". Essentially, this means that it must be possible to destroy the object in the normal way (by delete or by going out of scope, depending on how it was created).

So - provided your extractData does what it should and leaves the object in a state that allows it to be destructed later - there is nothing undefined or dangerous going on with respect to the compiler. There could of course be a problem with users of the code being confused, since it is not entirely obvious that the object is being moved from (and likely won't contain any data later). This could perhaps be made a little clearer by changing the function name. Or (as another answer suggested) by &&-qualifying the entire method.

Answer 4

I think you shouldn't move from *this, but from its data field. Since this clearly will leave the Marshaller object in an valid but unusable state, the member function that does this should itself should have an rvalue reference qualifier on its implicit *this argument.

class Marshaller
{
public:
  ...
  DataType Marshaller::unwrap() &&   { return std::move(data); }

  ...
private:
  DataType data;
};

Call it, if m is Marshaller variable, as std::move(m).unwrap(). There is not need for any static member to accomplish this.

Answer 5

You write that you'd like to simultaneously destroy the Marshaller and remove the data from it. I really wouldn't worry about trying to do these things simultaneously, just move the data out first and then destroy the Marshaller object. There are a number of ways to get rid of the Marshaller without thinking much about it, perhaps a smart pointer makes sense for you?

One option for refactoring this would be to give DataType a constructor that takes a Marshaller and moves the data out (the 'friend' keyword will allow you to do this since the DataType will then be able to reach that private 'data' variable).

    //add this line to the Marshaller
    friend class DataType;

struct DataType
{
    DataType(Marshaller& marshaller) {
             buffer = marshaller.data.buffer;
        }

    private: 
        Type_of_buffer buffer;//buffer still needs to know how to have data moved into it
}

You could also give it an assignment operator that does that same (I think this will just work:

DataType& operator=(Marshaller&& marshaller) {
     this.buffer = std::move(marshaller.data.buffer);
     return *this;
}

)

I would avoid using move on *this, just because it's going to throw people off even if it is right. It also seems like stack based buffer containers might get you into trouble.

You seem to be concerned with the Marshaller being called again outside the compilation unit. If you have intense parallel code and are playing fast and loose with the marshaller, or you are copying pointers to your marshaller willy-nilly then I think your worries are justified. Otherwise, look at how the Marshaller is being moved around and make sure you've structured your code for good object lifetime (use object references whenever you can). You can also just add a member flag to marshaller that says whether 'data' has been moved and throw an error if someone tries to access it after it's left (if you're parallel be sure to lock). I would only do this as a last resort or quick fix since it doesn't seem right and your co-devs will wonder what's up.

I have some nits to pick if you have a moment:

• your extractData method is missing the static keyword
• you mix brackets and parenthesis on your DataType declartion line