9

I need some help with C++ vectors. I have an object which has two attributes, both constant references (this means that I don't have an empty constructor for the class). I need to create a vector with some of these objects and I need to sort them based on the value of one of the attributes (specifically the otherObj attribute, which class has all the operators overloaded).

I'm having some trouble because, when I try to do a simple std::sort on the vector, I think it tries to create another object of that class with an empty constructor to swap those objects, so I don't know what to do.

I paste an example code of how I basically have these objects (I may have made some errors)

class Object{
  const OtherObject& otherObj;
  const int& myInt;

  Object(const OtherObject& o, const int& i){
    this->otherObj = o;
    this->myInt = i;
  }

};

void myFunction(std::vector<OtherObject>& vec){
  int i1 = 1;
  int i2 = 2;
  int i3 = 3;

  Object obj1(vec.at(0), i1);
  Object obj2(vec.at(1), i2);
  Object obj3(vec.at(2), i3);

  std::vector<Object> myVec {obj1, obj2, obj3};

  //Sort the vector here
}
Uwe Keim
  • 39,551
  • 56
  • 175
  • 291
Fsanna
  • 347
  • 1
  • 4
  • 11
  • Might not be relevant, but what's `OtherObject`? – doctorlove May 22 '19 at 08:48
  • Might be trivial, but did you remember to overload operator< in 'Object' for std::sort to work or do you use a custom sort function? – nada May 22 '19 at 08:50
  • @doctorlove it's an object in which I have overloaded almost all the operators, the sorting needs to be done based on those OtherObjects in an ascending order. I'll add it in the question – Fsanna May 22 '19 at 08:51
  • 7
    You can't re-seat references, and std::sort is going to try to swap (or move) your object. How would you expect that swap to work? (Generally you'd want to avoid reference members; see e.g. https://stackoverflow.com/a/892303/147845 and https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#define-copy-move-and-destroy-consistently) – You May 22 '19 at 08:53
  • 1
    are you sure your object has to save **references** ? – bruno May 22 '19 at 08:54
  • @nada I tried both, the latter with a simple std::iter_swap and that doesn't work either (maybe I'm making some mistakes) – Fsanna May 22 '19 at 08:54
  • @Sosnos We would need to see the relevant code to check if there are mistakes. – nada May 22 '19 at 08:55
  • @You that's why I'm asking how to do that without swapping the objects, I need to find a way to sort them without the std::sort – Fsanna May 22 '19 at 08:56
  • @Sosnos I see, you should probably clarify that in your question. – nada May 22 '19 at 08:57
  • @Sosnos memorizing references as you do is very dangerous, are you sure you want to do that ? it is so easy for the referenced elements to disapear – bruno May 22 '19 at 09:00
  • @bruno yes I need to use memory as less as possible and I've been told not to use pointers when not strictly necessary – Fsanna May 22 '19 at 09:00
  • @Sosnos: Sorting will necessarily move the objects around. This is by no means trivial to do with reference members. Do you really need reference members? Did you read those two links, which both basically say "reference members are almost always wrong"? – You May 22 '19 at 09:01
  • 3
    @Sosnos if the goal is just to save memory if you are on a 64b generaly the size of an _int_ is 32b while the size of an _int&_ is 64b so you **lost** memory (out of the danger to proceed as you do, your code is a factory of undefined behavior ;-) ) – bruno May 22 '19 at 09:01
  • @You yes I read them, basically I need to get rid of those references, but then how can I avoid to make copies of objects? – Fsanna May 22 '19 at 09:07
  • @bruno well that I didn't know, I'll change it, thank you! – Fsanna May 22 '19 at 09:08
  • 1
    Note that your constructor has undefined behaviour since you're assigning to uninitialised references. You must initialise references in the initialiser list. – molbdnilo May 22 '19 at 10:57

1 Answers1

11

std::sort requires ValueSwappable of the iterators passed to it, and MoveAssignable and MoveConstructible of the pointed-to type.

By default that will call std::swap, which is approximately

template< class T >
void swap( T& a, T& b )
{
    T temp = std::move(a);
    a = std::move(b);
    b = std::move(temp);
}

I.e. it will modify the objects such that they have the values each other started with. Your objects can't be assigned to, so instantiating swap fails.

You can std::list::sort objects that aren't assignable, so instead

void myFunction(std::vector<OtherObject>& vec){
  int i1 = 1;
  int i2 = 2;
  int i3 = 3;

  Object obj1(vec.at(0), i1);
  Object obj2(vec.at(1), i2);
  Object obj3(vec.at(2), i3);

  std::list<Object> myList{obj1, obj2, obj3};
  myList.sort();
  std::vector<Object> myVec{ myList.begin(), myList.end() };

}
Caleth
  • 52,200
  • 2
  • 44
  • 75