Segmentation fault with accessor function in C++

Question

I am working on a homework assignment for a C++ class, and having some difficulties. My program is segfaulting due to "use of uninitialised value of size 8" and "invalid read of size 8" in item::name() which is called by item::operator=.

#ifndef ITEM_H
#define ITEM_H

const double WEIGHT_DEFAULT = 1.0;
const int ITEM_NAME_LENGTH = 30;

class item {
public:
    // Constructors and deconstructor
    item();
    item(char* nme, double weight);
    virtual ~item();

    // Copy constructor
    item(const item& itm);

    // Overload assignment operator
    const item& operator=(const item& itm);

    // Overload inequality operator
    bool operator!=(const item&) const;

    // Mutators and accessors
    const char* name(void) const;
    double weight(void) const;
    void weight(double wght);
    void name(char* nme);

protected:
private:
    char* m_name;
    double m_weight;
};

#endif // ITEM_H

Here is the portion of the implementation that I think is having issues:

... 

const item& item::operator=(const item& itm)
{
    if (strcmp(this->name(), itm.name()) != 0)
    {

        this->weight(itm.weight());
        strcpy(m_name, itm.name());
    }

    return *this;
}

const char* item::name(void) const {
    return m_name;
}

...

As you can see, part of the assignment is to use c-strings, thus I'm stuck with the messy details. My understanding of pointers and classes is that when working with both, we need to implement destructors, copy constructors, and overload the assignment operator. I've done all of the above. I looked at some info on the copy-and-swap idiom here on Stack Overflow, and tried implementing that, but couldn't get that to work with my code either.

My brain is getting frazzled trying to puzzle out where I went wrong. Could someone please tell me what I'm missing?

Thanks!

---

EDIT

Here are the constructors, destructor, and such:

item::item() {
    //ctor

    m_name = new char[ITEM_NAME_LENGTH];
    strncpy(m_name, " ", ITEM_NAME_LENGTH - 1);
    this->weight(WEIGHT_DEFAULT);

    return;
}

item::item(char* nme, double wght) {
    m_name = new char[ITEM_NAME_LENGTH];

    strncpy(m_name, nme, ITEM_NAME_LENGTH - 1);

    // We want to check for a rational value being
    // passed in to weight. In theory, an item has
    // to have *some* kind of weight, so we check
    // for a weight of 0. If a weight of 0 has been passed in,
    // we instead initialize the item's weight to the
    // default weight (set in the header file).
    if (wght > 0.0) {
        this->weight(wght);
    } else {
        this->weight(WEIGHT_DEFAULT);
    }

    return;
}

item::~item() {
    //dtor

    // TODO: We need to clean up any variables here.
    delete[] m_name;
}

item::item(const item& itm) {
    // copy ctor
    this->weight(itm.weight());
    strncpy(m_name, itm.name(), ITEM_NAME_LENGTH - 1);

}

const item& item::operator=(const item& itm)
{
    if (strcmp(this->name(), itm.name()) != 0)
    {

        this->weight(itm.weight());
        strcpy(m_name, itm.name());
    }

    return *this;
}

bool item::operator!=(const item& itm) const
{
    return (strcmp(m_name, itm.name()) != 0);
}

EDIT 2

Now that I've stopped trying to dynamically allocate the name c-string, I'm getting memory errors on the weight function...

double item::weight(void) const {
    return m_weight;
}

void item::weight(double wght)
{
    m_weight = wght;
}

Answer 1

First off, you don't need to explicitly return; from void statements.

Second: When creating a copy constructor, you need to allocate the space that will be used to hold the memory of that constructor, the same way you do for the regular constructor.

Try this in your copy constructor:

m_name = new char[ITEM_NAME_LENGTH];

Edit: for completeness this is what my "appears to be working" code is:

#include <stdio.h>
#include <string.h>


const double WEIGHT_DEFAULT = 1.0;
const int ITEM_NAME_LENGTH = 30;

class item {
public:
    // Constructors and deconstructor
    item();
    item(char* nme, double weight);
    virtual ~item();

    // Copy constructor
    item(const item& itm);

    // Overload assignment operator
    const item& operator=(const item& itm);

    // Overload inequality operator
    bool operator!=(const item&) const;

    // Mutators and accessors
    const char* name(void) const;
    double weight(void) const;
    void weight(double wght);
    void name(char* nme);

protected:
private:
    char* m_name;
    double m_weight;
};


const item& item::operator=(const item& itm)
{
    if (strcmp(this->name(), itm.name()) != 0)
    {

        m_name = new char[ITEM_NAME_LENGTH];
        this->weight(itm.weight());
        strcpy(m_name, itm.name());
    }

    return *this;
}

const char* item::name(void) const {
    return m_name;
}

double item::weight(void) const {
    return m_weight;
}

void item::weight(double wght) {
    m_weight = wght;
}

item::item() {
    //ctor

    m_name = new char[ITEM_NAME_LENGTH];
    strncpy(m_name, " ", ITEM_NAME_LENGTH - 1);
    this->weight(WEIGHT_DEFAULT);

    return;
}

item::item(char* nme, double wght) {
    m_name = new char[ITEM_NAME_LENGTH];

    strncpy(m_name, nme, ITEM_NAME_LENGTH - 1);

    // We want to check for a rational value being
    // passed in to weight. In theory, an item has
    // to have *some* kind of weight, so we check
    // for a weight of 0. If a weight of 0 has been passed in,
    // we instead initialize the item's weight to the
    // default weight (set in the header file).
    if (wght > 0.0) {
        this->weight(wght);
    } else {
        this->weight(WEIGHT_DEFAULT);
    }

    return;
}

item::~item() {
    //dtor

    // TODO: We need to clean up any variables here.
    delete[] m_name;
}

item::item(const item& itm) {
    // copy ctor
    this->weight(itm.weight());
    m_name = new char[ITEM_NAME_LENGTH];
    strncpy(m_name, itm.name(), ITEM_NAME_LENGTH - 1);

}

bool item::operator!=(const item& itm) const
{
    return (strcmp(m_name, itm.name()) != 0);
}


int main(int argc, char* argv[])
{
    item i("test",2.0);
    item b = i;
    item c;
    item d = c;
    return 0;
}

Answer 2

I guess using std::string is out as part of the exercise, right? Otherwise, that's the way to go for string handling. Another thing you could try is to avoid the dynamic allocation of the string. Instead, just use a char m_name[ITEM_NAME_LENGTH];. BTW: Which length is that? If it's the maximum number of characters in the name, you need a +1 for the terminating NUL.

That said, one more thing: In assignment operators, you compare if(this != &other), i.e. you compare the address of the two objects. The point is to avoid self-assignment, which is error-prone. Your assignment checks if the names are different and only then copies the names and the weight. What if only the weight was different?