CODEWITHSUNDEEP
c++stdmap
cooooookie monster
cooooookie monster

Reputation: 334

c++ inseting User Defined class into map

I've been looking around for quite some time and think I have most of the pieces together but my code still won't work...

I have a map,

    map<Number, Entry> chainList;

and a class Number and Entry, the Entry we wont worry about for now as I'm pretty sure that half works correct

in Number.h
class Number
{
    public:

    //Public Functions
        //Constructor/Destructor
        Number(int len);
        Number(string copy);
        Number(const unsigned char *copy, int len);
        Number(const Number& in);
        ~Number();

        .......
        .......

        friend void swap(Number& first, Number& second);    

        bool operator<(const Number& rhs) const;
        Number& operator=(Number &rhs);

        private:
            //our binary number array
            unsigned char *num;
            //hold the length used, and maxsize of the array
            int length;
};

then,

//in Number.cpp
Number::~Number()
{
    delete [] num;  
}

Number::Number(const Number& in)
{
length = in.length;
num = (unsigned char *) calloc(length, sizeof(unsigned char));

    for (int i = 0; i < length; i++)
    {
        num[i] = in.num[i];
    }   
}

bool Number::operator<(const Number& rhs) const
{
    if (this -> length > rhs.length)
    {
        return false;
    }

    for (int i = 0; i < this -> length; i++)
    {
        if (this -> num[i] > rhs.num[i])
        {
            return false;
        }
        else if (this -> num[i] < rhs.num[i])
        {
            return true;
        }
    }

    return false;
}

void swap(Number& first, Number& second)
{
        // enable ADL (not necessary in our case, but good practice)
       using std::swap;

        // by swapping the members of two classes,
        // the two classes are effectively swapped
        swap(first.length, second.length);
        swap(first.num, second.num);
}


Number& Number::operator=(Number &rhs)
{
    swap (*this, rhs);
    return *this;
}

however when I try and insert an item into the map I get a seg fault....

in Database.cpp
....
chainList.insert(pair<Number, Entry>(*(tempEntry -> msgHash),  *tempEntry));
.....

where tempEntry -> msgHash is a Number* - dynamically allocated

what could my issue be? another option is I have a function that typecasts and returns a c++ string, my question is will the std::less_than function work with null characters in the middle of the statement, I know it works in lexigraphical order but is it up to the first null?

Upvotes: 1

Views: 45

Answers (1)

rodrigo
rodrigo

Reputation: 98348

I think the problem is in operator<():

if (this->length > rhs.length)
    return false;
for (int i = 0; i < rhs.length; i++)
    ....

See? If rhs.length is greater than this->length you go on and compare the bytes. But you compare up to rhs.length bytes, and that might overflow this->num, as this->length is less or equal to rhs.length.

I'm not sure if you need a specific sorting order, but I would do something like:

if (this->length > rhs.length)
    return false;
if (this->length < rhs.length)
    return true;
for (int i = 0; i < rhs.length; i++)
    ....

Now, when you reach the loop you are sure that both arrays are the same length.

UPDATE:

You have another important issue in operator=(Number &rhs). This operator should never modify the right-hand-side operator. So it should be operator=(const Number &rhs) or operator=(Number rhs), but never a non-const reference, as yours is.

You are trying to implement the copy-and-swap idiom. You got it almost right, the proper way is:

Number& Number::operator=(Number rhs)
{
    swap (*this, rhs);
    return *this;
}

UPDATE #2:

You are allocating your array with calloc() but freeing it with delete[]. That is undefined behaviour. Memory allocated with calloc() is freed with free(), and memory allocated with new[] is freed with delete[].

My advice is to use std::vector<unsigned char> to hold dynamic arrays, and avoid all the this->length, delete[], etc. Just do std::swap() on the vectors and it's done.

Upvotes: 2

Related Questions