If you plan on keeping the parameter anyway, then there’s no need to have separate T const& and T&& overloads

Raymond Chen

Raymond

Suppose you have a function that takes a std::vector and it wants to take ownership of the vector. You might decide to write two overloads:

class Widget
{
public:
  void SetValue(std::vector<int> const& value)
  {
    m_values = values;
  }

  void SetValue(std::vector<int>&& value)
  {
    m_values = std::move(values);
  }
private:
  std::vector<int> m_values;
};

If the caller passes a read-only lvalue reference, then you have to copy the vector. But if the caller passes an rvalue reference, then you can steal the vector by moving it into the m_values.

But really, the extra overload is just duplicated code for no real purpose. You’re going to keep the object either way, so just make the caller provide it in a form you can keep: Pass it by value.

class Widget
{
public:
  void SetValue(std::vector<int> value)
  {
    m_values = std::move(values);
  }

private:
  std::vector<int> m_values;
};

If the caller tries to pass an lvalue reference, then the compiler will use the copy constructor to create the inbound value parameter, which you then move into m_values.

If the caller tries to pass an rvalue reference, then the compiler will move it into the inbound value parameter, which you then move onward into m_values.

You get the same result without needing any overloads. Lvalues are copied and rvalues are moved.¹

Related reading: Rvalue references in constructor: when less is more.

¹ Now, there is a bit of extra cost associated with this simplification: The copying or moving into the parameter is done at the call site, and the formal parameter now needs to be destructed. But you avoid having to write two versions of every function, and I think that simplification is worth it. (Besides, the compiler can often optimize the move.)

13 comments

Comments are closed.

  • Avatar
    Phil Deets

    One thing to be aware of here is that this prevents buffer reuse for the buffer within m_values. The move will always end up taking the buffer from the parameter and freeing the existing buffer in m_values, rather than copying the ints over to the existing buffer.

    • Avatar
      Em X

      That’s just an implementation detail, not part of the contract. The contract here is that SetValue takes ownership of the passed-in value, and the post shows that there’s a perfectly correct way to do so without needing two different functions. If you want to copy the information into your existing buffer, you’re free to do so, instead of just taking the new one.

      • Avatar
        Phil Deets

        My point is that if the caller passes you an lvalue, you will end up having one additional allocation over what would happen if you used vector’s assignment operator for an lvalue reference. With this:

        void SetValue(std::vector const& value)
        {
        m_values = values;
        }

        The implementation of vector will internally copy the ints if m_values has a buffer which is big enough to accommodate the values from the parameter. When you do this:

        void SetValue(std::vector value)
        {
        m_values = std::move(values);
        }

        You are forcing a new buffer to be allocated to create the parameter. That buffer is then moved into m_values and the original buffer within m_values is freed. There will never be buffer reuse for the case when the data is set from an lvalue reference. If SetValue is called frequently, that can be a significant performance regression.

        The second function is perfectly correct, but the performance loss can be much higher than just the loss from the extra move.

        • Avatar
          Em X

          Sorry, I didn’t mean that the ABI was the contract, just the API (and perhaps not even then) if you can change arguments. Where performance is necessary, simplicity and clarity often have to be sacrificed.

          If the ABI is a contract, then you have a lot more up-front decisions to make about where performance is necessary, or just shoot for performance in all cases even if it’s a bit more complex to write and maintain. Or break un-recompiled callers on version upgrades.

  • Avatar
    Ismo Salonen

    For vectors this might be good thing, depending on std:.vector implementation. But for strings with small buffer optimizations the small strings are copied once more than when having two overloads. Depending how time sensitive your application is this might be bad thing. Personally I’ll write both overloads usually, not always.