14

Say I have this class:

class Message
{
public:
    using Payload = std::map<std::string, boost::any>;

    Message(int id, Payload payload)
    : id_(id),
      payload_(std::move(payload))
    {}

    int id() const {return id_;}

    const Payload& payload() const {return payload_;}

private:
    int id_;
    Payload payload_;
};

where Payload could potentially be large and expensive to copy.

I would like to give users of this Message class the opportunity to move the payload instead of having to copy it. What would be the best way to go about doing this?

I can think of the following ways:

  1. Add a Payload& payload() overload which returns a mutable reference. Users can then do:

    Payload mine = std::move(message.payload())

  2. Stop pretending that I'm encapsulating payload_ and just make it a public member.

  3. Provide a takePayload member function:

    Payload takePayload() {return std::move(payload_);}

  4. Provide this alternate member function:

    void move(Payload& dest) {dest = std::move(payload_);}

  5. (Courtesy of Tavian Barnes) Provide a payload getter overload that uses a ref-qualifier:

    const Payload& payload() const {return payload_;}

    Payload payload() && {return std::move(payload_);}

Alternative #3 seems to be what's done in std::future::get, overload (1).

Any advice on what would be the best alternative (or yet another solution) would be appreciated.


EDIT: Here's some background on what I'm trying to accomplish. In my real life work, this Message class is part of some communications middleware, and contains a bunch of other meta data that the user may or may not be interested in. I had thought that the user might want to move the payload data to his or her own data structures and discard the original Message object after receiving it.

Community
  • 1
  • 1
Emile Cormier
  • 28,391
  • 15
  • 94
  • 122
  • You could also make `Payload` an object that is copy on write, and have it implement some reference counting. This is what `std::string` used to do in some implementations. – Bill Lynch Apr 11 '15 at 00:04
  • 1
    "Best" in general seems a bit subjective. Alternative #3 seems best to me in terms of readability. Also, why not just implement/use move semantics for the whole `Message` class? – BartoszKP Apr 11 '15 at 00:05
  • 1
    I think that highly depends on **why** the user would want to move (or, in fact, even *access*) the member. Why is this class not the owner? – Konrad Rudolph Apr 11 '15 at 00:06
  • @BillLynch I deliberately made `Payload` a typedef of `std::map` to indicate that I have no control over it. That is the same situation in the real life problem I'm working on. – Emile Cormier Apr 11 '15 at 00:07
  • Option 3 is answered here http://stackoverflow.com/questions/14856344/when-should-stdmove-be-used-on-a-function-return-value – Steephen Apr 11 '15 at 00:10
  • 2
    @Steephen What does that question have to do with this? This is moving from a class member; it's got to be explicit. – T.C. Apr 11 '15 at 00:21
  • 3
    Another choice is `Payload payload() && {return std::move(payload_); }` – Tavian Barnes Apr 11 '15 at 00:23
  • 1
    @Steephen: I believe in that other question, the motivation was to prevent a temporary copy. In mine, I deliberately want the user to be able to move the private member variable, so that it's permanently "gone" from the object that originally hosted it. – Emile Cormier Apr 11 '15 at 00:42
  • @TavianBarnes: I'm leaning towards your suggestion, with #3 as a fallback on Visual Studio, which does not yet support ref-qualifiers. Can you post an answer? – Emile Cormier Apr 11 '15 at 00:49
  • I think, options 1 and 2 are somewhat inferior: you are encapsulating payload (not just pretending). Both options would allow to modify (set) the payload_ member (and such semantics is clearly not a part of Message's interface). Other options look better. Here is another possible solution: provide a class Holder which would hold just the payload. Make it a friend of Message and implement the constructor (or other method) taking Message&, which would move the payload out of it (and possibly perform other cleanup). Clients will then use the Holder to access payload and can discard Message. – Mikhail Maltsev Apr 11 '15 at 01:07
  • @MikhailMaltsev: What you propose would work, but it seems like an elaborate way to do the same as #3. – Emile Cormier Apr 11 '15 at 01:14

2 Answers2

3

It seems the most consistent approach is to use option 5:

  1. The 1st and 2nd option don't seem to be advised as they expose implementation details.
  2. When using a Message rvalue, e.g., a return from a function, it should be straight forward to move the payload and it is using the 5th option:

    Messsage some_function();
    Payload playload(some_function().payload()); // moves
    
  3. Using std::move(x) with an expression conventially indicates that the value of x isn't being depended upon going forward and its content may have been transferred. The 5th option is consistent with that notation.

  4. Using the same name and having the compiler figure out whether the content can be moved makes it easier in generic contexts:

    template <typename X>
    void f(X&& message_source) {
        Payload payload(message_source.get_message());
    }
    

    Depending on whether get_message() yields an lvalue or an rvalue the payload is copied or moved appropriately. The 3rd option doesn't yield that benefit.

  5. Returning the value make it possible to use the obtained in a context where copy-elision avoids further potential copies or moves:

    return std::move(message).payload(); // copy-elision enabled
    

    This is something the 4th option doesn't yield.

On the negative side of the balance sheet it is easy to incorrectly attempt moving the payload:

return std::move(message.payload()); // whoops - this copies!

Note, that the other overload for the 5th option needs be to be declared differently, though:

Payload        payload() &&     { return std::move(this->payload_); }
Payload const& payload() const& { return this->payload_; }
          // this is needed --^
Dietmar Kühl
  • 150,225
  • 13
  • 225
  • 380
  • 1
    If I understand correctly, with option 5, the client should be able to do this, `Payload mine = std::move(message).payload();` which will effectively move the payload from `message` to `mine`. This example is effectively the same as what's happening in the example you present in your item #2. Is this correct? – Emile Cormier Apr 11 '15 at 02:02
  • @EmileCormier: assuming there is a `()` after `payload`: yes. – Dietmar Kühl Apr 11 '15 at 02:06
  • Oops, yes. There was supposed to be a `()` after `payload`. Fixed my comment. – Emile Cormier Apr 11 '15 at 02:07
  • Dietmar, in the absence of ref-qualifier support (for example, on VS2013), do you think that my option #3 would be the "lesser of all evils"? – Emile Cormier Apr 11 '15 at 02:10
  • That's a different question... Depending on the needs the 4th option may be better as it gives a consistent interface for the generic case. If only one object is being returned copies can actually still be elided although it is necessary to create a `Payload` object which being reset. – Dietmar Kühl Apr 11 '15 at 02:31
3

The first thing I'd recommend is don't do this at all, if you can help it. Allowing your private data members to be moved from breaks encapsulation (even worse than returning const references to them). In my ~35,000 line codebase I have needed to do this precisely once.

That being said, I did need to do it once, and there were measurable and significant performance advantages to doing it. Here's my opinion on each of your suggested approaches:

  1. Add a Payload& payload() overload which returns a mutable reference. Users can then do:

    Payload mine = std::move(message.payload())

The downside here is that users can do message.payload() = something else;, which can mess with your invariants.

  1. Stop pretending that I'm encapsulating payload_ and just make it a public member.

Encapsulation isn't an all-or-nothing thing; IMO you should strive for as much encapsulation as is possible, or at least reasonable.

  1. Provide a takePayload member function:

Payload takePayload() {return std::move(payload_);}

This is my favourite solution if you don't have access to ref-qualifiers (seriously VC++?). I would probably name it movePayload(). Some may even prefer it to option 5 because it is more explicit and less confusing.

  1. Provide this alternate member function:

void move(Payload& dest) {dest = std::move(payload_);}

Why use an out parameter when a return value will do?

  1. (Courtesy of Tavian Barnes) Provide a payload getter overload that uses a ref-qualifier:

const Payload& payload() const {return payload_;}

Payload payload() && {return std::move(payload_);}

Somewhat unsurprisingly, this is my favourite suggestion :). Note that you'll have to write

const Payload& payload() const& { return payload_; }
Payload payload() && { return std::move(payload_); }

(const& instead of const) or the compiler will complain about ambiguous overloads.

This idiom is not without some caveats, although I believe Scott Meyers suggests it in one of his books so it can't be too bad. One of them is that the caller syntax is odd:

Message message;
Payload payload = std::move(message).payload();

It would be even worse if you needed to move two values out of a Message; the double std::move() would be very confusing to someone unfamiliar with the pattern.

On the other hand, this comes with a lot more safety than other methods, because you can only move from Messages that are seen to be xvalues.

There is a minor variation:

Payload&& payload() && { return std::move(payload_); }

I'm really not sure which one is better. Due to copy elision, the performance of both should be the same. They do have different behaviour when the return value is ignored.

Tavian Barnes
  • 12,477
  • 4
  • 45
  • 118
  • I'd like to heed your warning to not do this at all, but this is for a library, and I don't want to dictate how they should work with the message payload. Nor do I want to impose the cost of copying on them. Fortunately, once they receive this `Message` object, they can do whatever they want with it, and it will not affect the communications middleware at all. The thing that sucks about writing libraries is that you cannot make broad assumptions about usage patterns! – Emile Cormier Apr 11 '15 at 03:12
  • 1
    _although I believe Scott Meyers suggests it in one of his books so it can't be too bad_... I found it in Item 12 in Effective Modern C++. – Emile Cormier Apr 11 '15 at 15:23