0

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.

Sumit Jha
  • 1,601
  • 11
  • 18
  • 6
    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 May 04 '18 at 11:18
  • Why is your assignment operator trying to `swap` with `other`? It should absolutely *not* modify the rhs – Cory Kramer May 04 '18 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 May 04 '18 at 11:32
  • You should make `m_data` a `vector`. The assignment operator should include a test for `this == &other` to do nothing in the case of self assignment. – Khouri Giordano May 04 '18 at 11:39

2 Answers2

2

There can be some type of declarations copy assignment operator.

  1. It is typical declaration of a copy assignment operator when copy-and-swap idiom can be used:

    MyData& operator=(MyData other);
    
  2. 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);

YSC
  • 38,212
  • 9
  • 96
  • 149
Alexey Usachov
  • 1,364
  • 2
  • 8
  • 15
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;
}
mystic_coder
  • 462
  • 2
  • 10
  • *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. – WhozCraig May 04 '18 at 12:11
  • This is not exception safe. – Sumit Jha May 04 '18 at 12:20
  • I didn't intentionally made it exception safe . I just fixed compilation issue – mystic_coder May 04 '18 at 12:23
  • A blank source file would also fix the compilation issue, it is not useful though. – YSC May 04 '18 at 12:24