0

This question discusses passing by value vs. passing by rvalue reference in C++. However I find the answers unsatisfactory and not entirely correct.

Let's say I want to define a Queue abstract interface:

  • Queue is templated on the object it accepts
  • It should be possible to store noncopyable types in the Queue
  • The Queue may have multiple implementations.
  • Queue implementations should not be precluded from the opportunity of avoiding copies, if they can avoid them.

I wish for my interface to express the intent that calling Queue<T>::push(value) implies the "transfer" of value to the queue. By "transfer" I mean that:

  1. Queue::push is allowed to do whatever it wants with value, including modifying it, and the user should not be affected.
  2. After a call to Queue::push, if the user uses value, it should not have side effects for the Queue.

My options for the Queue interface are:

Option 1:

template<typename T>
class Queue {
public:
   virtual void push(const T& value) = 0;
};

Option 2:

template<typename T>
class Queue {
public:
   virtual void push(T&& value) = 0;
};

Option 3:

template<typename T>
class Queue {
public:
   virtual void push(T value) = 0;
};

The C++ core guidelines don't help much:

  • F.16 says "For “in” parameters, pass cheaply-copied types by value and others by reference to const". Here "value" is an in parameter, so the guidelines say I should pass it by reference to const. But this requires "T" to be copyable, which is an unnecessary restriction. Why shouldn't I be able to pass a std::unique_ptr to push?
  • F.18 says that "For “will-move-from” parameters, pass by X&& and std::move the parameter". First of all, what is a “will-move-from” parameter? Secondly, as I'm defining the Queue interface, I have no idea of what the implementation will do. It may move the object, or it may not. I don't know.

So what should I do?

  • Option 1: this does not express the right semantic IMO. With this option the code std::unique_ptr<int> ptr; queue.push(std::move(ptr)); does not work, but there's no reason why it shouldn't.
  • Option 2: this should have the right semantic, but it forces the user to either move or copy explicitly the value. But why should Queue force the user to do so? Why should for example Queue forbid the following code? std::shared_ptr<int> ptr; queue.push(ptr);
  • Option 3: it allows to push a copy of a value, or to "move-in" a value. So std::shared_ptr<int> ptr; queue.push(ptr); is valid, and so is std::unique_ptr<int> ptr; queue.push(std::move(ptr)); The required semantic holds. However, nothing stops the user from calling Storage<std::vector<int>>::store(veryLargeVector), which may cause a large, unnecessary copy.
Fabio
  • 644
  • 1
  • 8
  • 17
  • 2
    Why do you consider storing something taking over ownership? When you write a value to a file, `std::ofstream` doesn't take ownership of the value, it just stores a copy of it in the file. – NathanOliver Mar 21 '23 at 12:07
  • 1
    None of these three options transfer ownership. It can only be done with object with dynamic storage duration or some sort of resource. – user7860670 Mar 21 '23 at 12:09
  • your notion of "ownership" is odd. "ownership" has certain meaning and this is not it – 463035818_is_not_an_ai Mar 21 '23 at 12:09
  • btw if you find the answers to an existing question unsatisfactory then the right way to proceed is to write a better answer, write comments on the answers, or similar. Don't expect answers to be *better* by asking the same quesiton again – 463035818_is_not_an_ai Mar 21 '23 at 12:11
  • I think you misunderstand the meaning of "ownership". Take this example: `MyObject obj; Storage::store(obj)` What should happen with `obj`? Should `obj` live independently from the object in store? Should changes to `obj` reflect inside the storage and vice versa? – Raildex Mar 21 '23 at 12:16
  • 3
    You assume we understand what your Storage::store(x) is supposed to do in your imagination. You use word "serialize" but that means (to everybody I know of) that it needs no ownership nor copies whatsoever and therefore Option 1. But you tell something else. – Öö Tiib Mar 21 '23 at 12:24
  • 1
    `virtual void store(std::unique_ptr value) = 0;` is one way to take ownership. – Eljay Mar 21 '23 at 12:25
  • 1
    _in a modern codebase where most types are either move-only or cheap to copy_ Why would you draw that inference? – Paul Sanders Mar 21 '23 at 12:39
  • Passing by value (option 3) let's the caller decide whether move or copy is more appropriate. – Richard Critten Mar 21 '23 at 12:50
  • "the guidelines say I should pass it by reference to const. But this requires "T" to be copyable" [this is not true in terms of the interface](https://godbolt.org/z/ev6nz4efj). That *might* be the case for the implementation, but without it provided it cannot be asserted. – alagner Mar 21 '23 at 12:50
  • Anyway, I believe "storage" with multiple implementations is not the best example of ownership transfer, as serialization generally does not do it. It seems reimplementing sth like std::optional could suit better here. But all that aside: why not use templates and SFINAE on type traits? – alagner Mar 21 '23 at 12:57
  • I've changed my question from a "Storage" to a "Queue", and clarified what I mean by "ownership transfer". This should address most comments – Fabio Mar 21 '23 at 13:44
  • @Fabio Now you seem to answer to your own "why?" question in Option 2 with requirement "After a call to Queue::push, if the user uses value, it should not have side effects for the Queue". The shared_ptr means shared ownership and so changing the object after push will change the object "owned" by queue. – Öö Tiib Mar 21 '23 at 14:34
  • @ÖöTiib Keep in mind that "value" in this context is the shared_ptr itself. Both with option 2 and 3, if the user modifies the shared_ptr (e.g. it reassigns it to something else) the shared_ptr in the queue is unaffected. – Fabio Mar 21 '23 at 14:45
  • @Fabio then keep in mind that moving shared_ptr's value is hundred times quicker than copying it because copy means incrementing its atomic reference count while move "steals" its value leaving empty shared_ptr to caller and so not incrementing counts. – Öö Tiib Mar 21 '23 at 16:06
  • @ÖöTiib Yes exactly. But the shared_ptr can be moved whether you use option 2 or option 3. With option 2 you *have* to call std::move, while with option 3 you are not forced to, but you can. So which one would you use in your code? – Fabio Mar 21 '23 at 17:02
  • @Fabio shared_ptr can be also copied regardless if you use option 2 or option 3. Only you can answer. If your push() is expecting private copy then accept by value, if it is expecting to take over then accept by rvalue reference. If you don't know then we don't know. Caller can anyway decide to move into value or to make a copy for passing as rvalue. You have to decide what is more normal/usual, so unusual usage is inconvenient and more visible. – Öö Tiib Mar 22 '23 at 09:02

1 Answers1

0

You expressed some concern about the following making an expensive copy:

queue.push(veryLargeVector);

But the problem is that there are only two things that this code can do. It can be made ill-formed, or it can copy veryLargeVector. Even if you take the argument by const reference (instead of by value) you will still need to make a copy from that reference into the queue's storage buffer.

There is no way for the queue template to detect whether or not it will be expensive to make a copy of the type it was instantiated with. You basically have to make the same choice for all types: should queue.push(x); make a copy or should it be ill-formed? And if you decide to make it ill-formed, then queue.push(sharedPtr) will also be ill-formed, but you wanted to support that.

And in fact queue.push(sharedPtr) should work. There is no widely-used generic container anywhere that only accepts rvalues (with the exception of ones that only store move-only types like std::unique_ptrs). If your API only accepts std::shared_ptrs and even ints by rvalue and not by lvalue, it will deviate from universal practice in the C++ community. Your users will find it highly annoying to have to write queue.push(int(x)) instead of queue.push(x) every time they want to copy an int into the queue. If you do not want to create this problem for users, you have to be willing to accept lvalues. And if you do accept lvalues, you have to copy from them. No other choice.

So there are really two options:

  1. Provide a pair of overloads: push(const T&) and push(T&&). This is what the standard library does.
  2. Provide a single overload, push(T). This means that with an lvalue argument, there will be a copy and a move, and with an rvalue argument, two moves.

If you expect almost all types that will be used with your queue to be ones that are cheaply movable, the second strategy will be more convenient.

Brian Bi
  • 111,498
  • 10
  • 176
  • 312
  • Another option is to make `push()` be a template with a [*forwarding reference*](https://en.cppreference.com/w/cpp/language/reference#Forwarding_references) of type `U`, using a concept/SFINAE to make sure that `U` is compatible with the class's main `T` type. This way, if the user passes in a `U` as an lvalue, it will be passed as `U&`, and if the user passes in a `U` as an rvalue, it will be passed as `U&&`. `push()` can use an `if constexpr` expression to detect which is used and act accordingly. You could then specialize `push()` to handle `unique_ptr` and `shared_ptr` as needed. – Remy Lebeau Mar 21 '23 at 23:28
  • "*[`push(T)`] means that with an lvalue argument, there will be a copy and a move, and with an rvalue argument, two copies.*" - that doesn't read right to me. Are you sure you wrote that correctly? Perhaps you meant that an lvalue argument will be a copy, and an rvalue argument will allow moving? – Remy Lebeau Mar 21 '23 at 23:29
  • @RemyLebeau It can't be a forwarding reference, because `virtual` functions can't be templates. – Brian Bi Mar 21 '23 at 23:30
  • You are right, I missed that detail. In which case, you could wrap the virtual method inside of a non-virtual template function. Have the template prepare a new queue item as needed from the input, and then have `push()` take ownership of that item. – Remy Lebeau Mar 21 '23 at 23:32
  • @BrianBi 1) you say "You want the following to either not make a copy, or refuse to compile". This is not what I said though... 2) you say that "It will be very annoying and unexpected for users if they cannot pass by value without std::move". Why? I'm not sure I agree with this statement 3) you suggest using two overloads. Isn't that a bit annoying, if the two overloads are pure virtual? 4) If we can assume that in the context of the application moves are cheap, would you go for option 1 or option 2 of the two you propose? – Fabio Mar 22 '23 at 16:16
  • @Fabio Then, what was the point of your remark about passing `veryLargeVector` (without `std::move`)? What do you want it to do? Clearly this must be relevant to your issue of what interface to provide, otherwise you wouldn't have mentioned it. But I am telling you that "make a copy" or "ill-formed" are the only options. – Brian Bi Mar 22 '23 at 18:39
  • "I am telling you that "make a copy" or "ill-formed" are the only options". Exactly! I 100% agree. My question is: which one is preferable? Which one would you use in your code? Would you allow the copy, or would you make it ill formed? – Fabio Mar 23 '23 at 12:38