The Sort method in STL cannot swap contents in a vector

Question

I was trying to implement Kruskal's algorithm (implementation of Introduction to Algorithms CLRS) using C++. But when trying to sort the Edge (which is the class i created) set (which i have implemented as a vector) using std::sort , it doesnt work. I tried using lambda function as comparator as well as overloaded '<' operator.

std::vector <Edge> givenEdgeSet;

This is the function call

std::sort(givenEdgeSet.begin(), givenEdgeSet.end());

And the overloaded operator definition is

bool operator < (const Edge& b){

//length is the edge length here..
        return (length < b.length);
    }

Problem appears to be in swapping of the objects in vector (which isn happening)

I have defined copy and move constructors and assignment operators Could it be a problem in the constructors ?

// copy constructor

    Edge(const Edge& e){
        start_point = e.start_point; 
        end_point = e.end_point;
        length = e.length;
        start_vertex_set = e.start_vertex_set;
        end_vertex_set = e.end_vertex_set;
    }

// copy assignment operator

    Edge& operator = (const Edge& e){
        std::shared_ptr<Edge> NewEdge(new Edge());
        NewEdge->start_point = e.start_point; 
        NewEdge->end_point = e.end_point;
        NewEdge->length = e.length;
        NewEdge->start_vertex_set = e.start_vertex_set;
        NewEdge->end_vertex_set = e.end_vertex_set;
        return *NewEdge;
    }

// Move Constructor

    Edge(const Edge&& e){
        start_point = e.start_point; 
        end_point = e.end_point;
        length = e.length;
        start_vertex_set = e.start_vertex_set;
        end_vertex_set = e.end_vertex_set;
    }

// Move assignment operator

    Edge& operator = (const Edge&& e){
        std::shared_ptr<Edge> NewEdge(new Edge());
        NewEdge->start_point = e.start_point; 
        NewEdge->end_point = e.end_point;
        NewEdge->length = e.length;
        NewEdge->start_vertex_set = e.start_vertex_set;
        NewEdge->end_vertex_set = e.end_vertex_set;
        return (*NewEdge);
    }

1) Please provide [mcve]. 2) What do you mean by "_cannot swap contents_"? 3) Both assignment operators return dangling reference, and if one uses that return value, one invokes undefined behavior. — Algirdas Preidžius, Jan 13 '19 at 10:34
Move constructor should accept non-const argument! How else would you want to move data out of? — Aconcagua, Jan 13 '19 at 10:37
If your members are of the types they seem to be, you don't need to implement any of those – the default behaviour will be correct. — molbdnilo, Jan 13 '19 at 10:37
You should prefer to use constructor's initialiser list (not to be confused with `std::initializerlist`!) over assignments in its body. — Aconcagua, Jan 13 '19 at 10:44

score 3 · Answer 1 · answered Jan 13 '19 at 10:35

3

Your assignment operators are a little unconventional and return references to objects which no longer exist.

The assignment operators are supposed to assign to the current object and return that object not a new one:

Edge& operator = (const Edge& e){
    start_point = e.start_point; 
    end_point = e.end_point;
    length = e.length;
    start_vertex_set = e.start_vertex_set;
    end_vertex_set = e.end_vertex_set;
    return *this;
}

You can omit your move constructor/operator as they are only copying data anyway so provide no benefit.

answered Jan 13 '19 at 10:35

Alan Birtles

32,622
4
31
60

7

I think "a little unconventional" is an understatement. – molbdnilo Jan 13 '19 at 10:38
Probably easier: `Edge& operator=(Edge const&) = default;` – Aconcagua Jan 13 '19 at 10:40
`[...]_vertex_set` - might be a `std::vector`, too. Then omitting move constructor wouldn't be that a good idea, would it? But default implementation would be fine then... – Aconcagua Jan 13 '19 at 10:42
Thank you... that solved the problem...i am still learning about moving objects and obviously that confused me hence the blunder – Sheikh Abdul Manan Jan 13 '19 at 10:45
@SheikhAbdulManan If you explicitly implement move constructor: do not forget to apply `std::move` to the moving object's members! – Aconcagua Jan 13 '19 at 10:46
@Aconcagua that i know...but i see a lot of comments saying my move constructor/operator is unconventional....how come ? – Sheikh Abdul Manan Jan 13 '19 at 10:49
@SheikhAdbulManan A move constructor does not have **const** specifier in its declaration... Look [here](https://en.cppreference.com/w/cpp/language/move_constructor)... This is the correct way to declare a move constructor... If you make the *rvalue* const, it won't be modifiable and that won't be a move at all... – Ruks Jan 13 '19 at 10:50
@SheikhAbdulManan In two ways: assignment should modify the object the operator is applied to - and not create a new one instead, as your's does. The return value is needed for doing stuff like `x = y = z;`, where `x` is assigned the return value (the result of `y = z`). Worse: your shared pointer runs out of scope and gets deleted - thus the object gets deleted. You return a reference, not a copy, thus the reference gets dangling. – Aconcagua Jan 13 '19 at 10:53
Hope you use `std::vector` or another STD container for your two vertex sets - if raw arrays, you'd need to explicitly deep copy instead of just assigning the pointer (and then, default implementation won't fit either). – Aconcagua Jan 13 '19 at 10:56
@Aconcagua so i should only use move when either an lvalue is no longer required and can be edited ( means no const )......and when providing an rvalue , do i still need std::move() ? – Sheikh Abdul Manan Jan 13 '19 at 10:58
@Aconcagua yes , my aim more than to implement kruskal is to understand STL ... im using STL as much as i can... there are no raw arrays...and im trying to avoid raw pointers – Sheikh Abdul Manan Jan 13 '19 at 11:00
@SheikhAbdulManan Avoiding raw pointers is *very* good practice. For understanding, now *imagine* you still used them. Each instance should contain its own - let's say array. On copy-assignment, you need to make a deep copy of the array, such that both objects contain their own instance, two separate copies. On moving, you assign the pointer to the target object - but the source object must not hold the pointer to the same array any more - so you need to set it e. g. to nullptr. To avoid memory leak, you need to delete the old array of the target object as well. – Aconcagua Jan 13 '19 at 11:04
@SheikhAbdulManan `std::vector` relieves you from all this work, it does all of it internally... – Aconcagua Jan 13 '19 at 11:07
@Aconcagua Thank you very much for your assistance. Learnt a lot – Sheikh Abdul Manan Jan 13 '19 at 11:11

The Sort method in STL cannot swap contents in a vector

1 Answers1