Cannot sort std::vector using lambda with const reference

Question

I want to sort a vector according to the custom datatype. I followed Sorting a vector of custom objects answer. I am using lambda function to compare the objects. However I am getting compiler errors as following while sorting:

/usr/include/c++/7/bits/stl_algo.h:1852: error: cannot bind non-const lvalue reference of type 'MyData&' to an rvalue of type 'std::remove_reference::type {aka MyData}' *__first = _GLIBCXX_MOVE(__val); ^

main.cpp

#include "mydata.h"
#include <vector>

int main()
{
    std::vector<MyData> tv {MyData(2,21), MyData(3,20), MyData(10,100), MyData(9,20)};

    std::sort(tv.begin(), tv.end(), []( MyData const& lhs, MyData const& rhs ){
         return lhs.get_size() < rhs.get_size();
    });

    return 0;
}

mydata.cpp

#ifndef MYDATA_H
#define MYDATA_H
#include <iostream>
#include <algorithm>

class MyData
{
private:

    int *m_data;
    int m_x;
    size_t m_size;

public:
    MyData(const size_t &size,int const &x):
        m_data(new int[size]),
        m_x(x),
        m_size(size)
    {

        std::fill_n(m_data,m_size, m_x);
        std::cout << *m_data << " ctor" << m_size << std::endl;
    }

    MyData(const MyData& other):
        m_data(new int[other.m_size]),
        m_x(other.m_x),
        m_size(other.m_size)
    {
        std::fill_n(m_data,m_size, m_x);
        std::cout << *m_data << " cctor" << m_size << std::endl;
    }

    MyData& operator=(MyData& other)
    {
        std::cout << *m_data << " cbctor" << m_size << std::endl;
        swap(*this,other);
        std::cout << *m_data << " cactor" << m_size << std::endl;
        return *this;
    }

    ~MyData(){
        std::cout << *m_data << " dtor" << m_size << std::endl;
        delete[] m_data;
    }

    size_t get_size() const{
        return m_size;
    }

    friend void swap(MyData& first, MyData& second){    // (1)
        std::swap(first.m_size, second.m_size);
        std::swap(first.m_x, second.m_x);
        std::swap(first.m_data, second.m_data);
    }

    friend std::ostream& operator<< (std::ostream& stream, const MyData& mydata) {
        stream << *(mydata.m_data) << " " << mydata.m_size << " "<< mydata.m_x;
        return stream;

    }

};

#endif // MYDATA_H

I do not understand the error. I am not changing the value of the reference, why I am getting this error. I also read this but did not understand why it is occurring here. Thank you.

Your assignment operator is very wrong. If you have e.g. MyData a(...), b(...); then a = b would swap a and b, but b is not expected to change. If you want to use swap in your assignment operator, pass the argument by value. Should incidentally solve your problem as well. — Some programmer dude
– Some programmer dude, Commented May 4, 2018 at 11:18
Why is your assignment operator trying to swap with other? It should absolutely not modify the rhs — Cory Kramer
– Cory Kramer, Commented May 4, 2018 at 11:20
Thanks @Someprogrammerdude changing assignment to MyData& operator=(MyData other) solved the problem. However I dont know if using swap is best in this case. I followed some blogs were they say using swap will make the assignment operator exception free. — Sumit Jha
– Sumit Jha, Commented May 4, 2018 at 11:32
You should make m_data a vector<int>. The assignment operator should include a test for this == &other to do nothing in the case of self assignment. — Khouri Giordano
– Khouri Giordano, Commented May 4, 2018 at 11:39

YSC · Accepted Answer · 2018-05-04 12:27:41Z

3

There can be some type of declarations copy assignment operator.

It is typical declaration of a copy assignment operator when copy-and-swap idiom can be used:
```
MyData& operator=(MyData other);
```
It is typical declaration of a copy assignment operator when copy-and-swap idiom cannot be used (non-swappable type or degraded performance):
```
MyData& operator=(const MyData& other);
```

So to use swap in your realization you can declare copy assignment operator as MyData& operator=(MyData other);

edited May 4, 2018 at 12:27

YSC

40.4k11 gold badges103 silver badges160 bronze badges

answered May 4, 2018 at 12:03

Alexey Usachov

1,3742 gold badges9 silver badges15 bronze badges

Sign up to request clarification or add additional context in comments.

Comments

mystic_coder · Accepted Answer · 2018-05-04 12:02:00Z

0

You should modify your code like this :

#include <iostream>
#include <fstream>
#include <thread>
#include <atomic>
#include <algorithm>
#include <vector>

using namespace std;
class MyData
{
private:

    int *m_data;
    int m_x;
    size_t m_size;

public:
    MyData(const size_t &size, int const &x) :
        m_data(new int[size]),
        m_x(x),
        m_size(size)
    {

        std::fill_n(m_data, m_size, m_x);
        std::cout << *m_data << " ctor" << m_size << std::endl;
    }

    MyData(const MyData& other) :
        m_data(new int[other.m_size]),
        m_x(other.m_x),
        m_size(other.m_size)
    {
        std::fill_n(m_data, m_size, m_x);
        std::cout << *m_data << " cctor" << m_size << std::endl;
    }

    MyData& operator=(const MyData& other)
    {
        std::cout << *m_data << " cbctor" << m_size << std::endl;
        //swap(*this, other);

        if (this != &other)
        {
            this->m_data = new int[other.m_size];
            for (size_t i = 0; i < other.m_size; ++i)
            {
                this->m_data[i] = other.m_data[i];
            }
            this->m_x = other.m_x;
            this->m_size = other.m_size;
        }
        std::cout << *m_data << " cactor" << m_size << std::endl;
        return *this;
    }

    ~MyData() {
        std::cout << *m_data << " dtor" << m_size << std::endl;
        delete[] m_data;
    }

    size_t get_size() const {
        return m_size;
    }

    friend void swap(MyData& first, MyData& second) {    // (1)
        std::swap(first.m_size, second.m_size);
        std::swap(first.m_x, second.m_x);
        std::swap(first.m_data, second.m_data);
    }

    friend std::ostream& operator<< (std::ostream& stream, const MyData& mydata) {
        stream << *(mydata.m_data) << " " << mydata.m_size << " " << mydata.m_x;
        return stream;

    }

};

int main()
{
    std::vector<MyData> tv{ MyData(2,21), MyData(3,20), MyData(10,100), MyData(9,20) };

    std::sort(tv.begin(), tv.end(), [](MyData const& lhs, MyData const& rhs) {
        return lhs.get_size() < rhs.get_size();
    });
    std::system("pause");
    return 0;
}

answered May 4, 2018 at 12:02

mystic_coder

4822 silver badges10 bronze badges

4 Comments

WhozCraig Over a year ago

Don't modify your code like this. The assignment operator in this code is a recipe for a memory leak. The statement this->m_data = new int[other.m_size]; orphans (and thus leaks) whatever original data this->m_data pointed to at function entry.

Sumit Jha Over a year ago

This is not exception safe.

mystic_coder Over a year ago

I didn't intentionally made it exception safe . I just fixed compilation issue

YSC Over a year ago

A blank source file would also fix the compilation issue, it is not useful though.

Collectives™ on Stack Overflow

Cannot sort std::vector using lambda with const reference

2 Answers 2

Comments

4 Comments

Your Answer

Linked

Hot Network Questions

Collectives™ on Stack Overflow

2 Answers 2

Comments

4 Comments

Your Answer

Sign up or log in

Post as a guest

Linked

Related