Custom object bad behaviour after std::sort

Question

I'm experiencing issues after sorting a std::vector of Elevator, a custom class.

Class definition:

class Elevator
{
private:

        uint16_t m_id;
        char *m_channel;
        ElevatorType m_type;

public:

        Elevator(uint16_t id, const char *channel, ElevatorType type);
        Elevator(const Elevator &other);
        ~Elevator();

        char *ToCString();

        bool operator<(const Elevator& rhs) const;

        static vector<Elevator> *ElevatorsFromWorkbook(const char *path);

};

The less operator is implemented like this:

bool Elevator::operator<(const Elevator& rhs) const
{
    return (this->m_id < rhs.m_id);
}

ToCString method is implemented like this:

char *Elevator::ToCString()
{
    char *output = (char*)malloc(sizeof(char) * (8+strlen(m_channel)));
    char type = 'U';
    if (m_type == ELEVATOR_TYPE_A)
    {
        type = 'A';
    }
    else if (m_type == ELEVATOR_TYPE_G)
    {
        type = 'G';
    } 
    else if (m_type == ELEVATOR_TYPE_M)
    {
        type = 'M';
    }
    sprintf(output,"%d %s %c",m_id,m_channel,type);
    return output;
}

Copy constructor:

Elevator::Elevator(const Elevator &other)
{
    m_id = other.m_id;
    m_channel = (char*)malloc(sizeof(char)*(strlen(other.m_channel)+1));
    strcpy(m_channel,other.m_channel);
    m_type = other.m_type;

}

Example of the issue:

std::vector<Elevator> elevators;
elevators.push_back(Elevator(4569,"CHANNEL3",ELEVATOR_TYPE_G));
elevators.push_back(Elevator(4567,"CHANNEL3",ELEVATOR_TYPE_G));

printf("%s\n",elevators.at(0).ToCString()); //Prints "4567 CHANNEL1 G"
std::sort(elevators.begin(),elevators.end());
printf("%s\n",elevators.at(0).ToCString()); //ISSUE: Prints "4567 4567 ▒#a G"

What am I doing wrong? Thanks for your time!

Note on compiling: I'm using Cygwin with these flags: -Wall -Wextra -fno-exceptions -fno-rtti -march=pentium4 -O2 -fomit-frame-pointer -pipe

Looks like the kind of trouble you get into when you manage memory with raw pointers (I suggest using `std::string` instead of a naked char*`). How is your copy constructor defined? — Andy Prowl, Jan 30 '13 at 23:43
Try replace all `char*` to `std::string`, `printf` with `std::cout`, mixing C/C++, miserable life starts — billz, Jan 30 '13 at 23:47
It's really likely that you're messing up with memory allocation and stuff. Try using `std::string` and you'll likely see these issues vanish — Andy Prowl, Jan 30 '13 at 23:48
@Zi0P4tch0: You also now need an assignment operator, see the [Rule of Three](http://stackoverflow.com/questions/4172722/what-is-the-rule-of-three). — GManNickG, Jan 30 '13 at 23:51
@GManNickG: you are right, sir! Implementing assignment operator solved the problem! Thanks! — Zi0P4tch0, Jan 31 '13 at 00:22
Or use std::string like everyone is recommending and ditch both your assignment operator and copy constructor. That, of course, assuming ElevatorType is copyable and assignable. Doing so lets you use compiler-genrated construction and assignment code (not to mention alleviate a lot of potential mistakes). — WhozCraig, Jan 31 '13 at 00:25

score 1 · Answer 1 · edited May 23 '17 at 10:31

According to the standard, std::sort requires that an object be Swappable, which means that you can call swap on it. Since you don't have a swap overload, that means std::swap. And std::swap says

Requires: Type T shall be MoveConstructible (Table 20) and MoveAssignable (Table 22).

(That's C++11. Replace "Move" with "Copy" for C++03.)

Your class is not CopyAssignable because it lacks an operator= overload, so you should add that. (See What is The Rule of Three?) Note that copy construction can be synthesized from default construction and operator=.

Custom object bad behaviour after std::sort

1 Answers1