Sorting a list of unique_ptrs

Question

The following code will not compile:

bool ptrLess(unique_ptr<int> ptr1, unique_ptr<int> ptr2)
{
   return *ptr1 < *ptr2;
}

int main()
{
   unique_ptr<int> ptr1(new int(3));
   unique_ptr<int> ptr2(new int(2));
   unique_ptr<int> ptr3(new int(5));
   list<unique_ptr<int>> list;

   list.push_back(ptr1);
   list.push_back(ptr2);
   list.push_back(ptr3);

   list.sort(ptrLess);

   for (auto &element : list) {
      cout << *element;
   }

   return 0;
}

I assume this is because unique_ptr's copy constructor is deleted. I get an error like:

error C2280: 'std::unique_ptr>::unique_ptr(const std::unique_ptr<_Ty,std::default_delete<_Ty>> &)': attempting to reference a deleted function

Is there any way to sort a list of unique_ptr's, perhaps by using the move constructor instead?

Answer 1

You should use const ref - after all you don't want to modify those pointers:

bool ptrLess(const unique_ptr<int>& ptr1, const unique_ptr<int>& ptr2)

If your list template is std::list, then passing arguments as r-value references won't work - list::sort would have to call std::move effectively resetting your pointers.

Edit

As for list the rest of your code: std::list has a handy method called emplace_back (and emplace_front) which allows you to construct and append an element in-place:

your_list.emplace_back(new int(2));

Answer 2

Try this:

#include <memory>
#include <list>
#include <iostream>
using namespace ::std;

bool ptrLess(unique_ptr<int>& ptr1, unique_ptr<int>& ptr2)
{
   return *ptr1 < *ptr2;
}

int main()
{
   unique_ptr<int> ptr1(new int(3));
   unique_ptr<int> ptr2(new int(2));
   unique_ptr<int> ptr3(new int(5));
   list<unique_ptr<int>> list;

   list.push_back(move(ptr1));
   list.push_back(move(ptr2));
   list.push_back(move(ptr3));

   list.sort(ptrLess);

   for (auto &element : list) {
      cout << *element;
   }

   return 0;
}

The problem here is that you need to understand what a unique_ptr actually aims to be:

When dealing with pointers/references, there are a hell lot of potential problems arising if there are several pointersrs/references referring to the same object. unique_ptr tries to avoid precisely that. Therefore, you cannot create 2 unique_ptr referring to the same object.

You cannot use your ptrLess() function because calling it like

   unique_ptr<int> ptr1(new int(3));
   unique_ptr<int> ptr2(new int(2));

   ptrLess(ptr1, ptr2);

because this would mean ptr1 an ptr2 would have to be copied and passed over to ptrLess() - keyword here is 'call-by-value'.

Moreover, you cannot do

   list<unique_ptr<int>> list;
   unique_ptr<int> ptr1(new int(3));

   unique_ptr<int> ptr1(new int(3));

because this too, would have to create a copy of ptr1. The solution here is to not pass the unique_ptr s to ptrLess as values, but as reference:

bool ptrLess(unique_ptr<int>& ptr1, unique_ptr<int>& ptr2);

And you dont pass copies into the list, but move your object there:

list.push_back(move(ptr1));

keyword here is 'move-semantics'. This will invalidate the content of your ptr1 variable - the object has been moved out of ptr1 into the list.

I recommend having a look at the Rust language if you are more deeply interested in things like these ;)

As Baum mit Augen pointed out, the parameters to ptrLess better be declared as const:

bool ptrLess(const unique_ptr<int>& ptr1, const unique_ptr<int>& ptr2);

Answer 3

Try passing by const ref so it won't copy the parameters : bool ptrLess(const unique_ptr& ptr1, const unique_ptr& ptr2) { return *ptr1 < *ptr2; }

Answer 4

It occurs to me to suggest that if the OP uses shared_ptr instead of unique_ptr then the code as originally posted will otherwise run unchanged:

#include <memory>
#include <list>
#include <iostream>
using namespace ::std;

bool ptrLess(const shared_ptr<int>& ptr1, const shared_ptr<int>&  ptr2)
{
   return *ptr1 < *ptr2;
}

int main()
{
   shared_ptr<int> ptr1(new int(3));
   shared_ptr<int> ptr2(new int(2));
   shared_ptr<int> ptr3(new int(5));
   list<const shared_ptr<int>> list;

   list.push_back(ptr1);
   list.push_back(ptr2);
   list.push_back(ptr3);

   list.sort(ptrLess);

   for (auto &element : list) {
      cout << *element;
   }

   return 0;
}

Run it on Wandbox.

In some senses, this is a consistent way to do things. push_back would normally copy the object being added to the list, leaving the original object still available to the caller should he or she want to use it. Using shared_ptr has similar semantics without the overhead of copying the object itself. Instead, just the shared_ptr is copied, which is a cheap operation.

Also, modifying the OP's original code to move the unique_ptrs into the list is inherently fragile. They remain in scope to the caller but are no longer usable. You will get (I assume) a nullptr dereference if you try. Better then, to do this (note the extra set of braces):

...

list<unique_ptr<int>> list;

{
   unique_ptr<int> ptr1(new int(3));
   unique_ptr<int> ptr2(new int(2));
   unique_ptr<int> ptr3(new int(5));

   list.push_back(move(ptr1));
   list.push_back(move(ptr2));
   list.push_back(move(ptr3));
}

...

Now you are safe.

There, that's a much better post than the original version, sorry about that.