c++ class pointer deletion segfaulting

Question

I've got a simple class called object that I'm having a problem with. Theres one method which causes a segfault if I call it. I don't understand why.

typedef class object
{
   private:
      short id;
      std::string name;
      SDL_Rect offset;

   public:
      object();
      object(short i, std::string n);
      ~object();
      object(const object &o);
      object& operator = (const object &o);
      std::string get_name();
      void set_name(std::string n);
} object;

object::object()
{
   id = 0;
   name = "";
   offset.x = 0;
   offset.y = 0;
   offset.w = 0;
   offset.h = 0;
}

object::object(short i, std::string n)
{
   id = i;
   name = n;
   offset.x = 0;
   offset.y = 0;
   offset.w = 0;
   offset.h = 0;
}

object::~object()
{
   delete &offset;
   delete &id;
   delete &name;
}

object& object::operator=(const object &o)
{
   if(this != &o)
   {
      delete &name;
      name.assign(o.name);
      delete &id;
      id = o.id;
      delete &offset;
      offset = o.offset;
   }
   return *this;
}

object::object(const object &o)
{
   id = o.id;
   name = o.name;
   offset = o.offset;
}

// Functions
std::string object::get_name()
{
   return name;
}

void object::set_name(std::string n)
{
   name = n;
}

And my main.cpp

int main( int argc, char** argv )
{
   struct object *a = new object(0, "test");
   struct object *b = new object(1, "another test");

   printf(a->get_name().c_str());
   printf("\n");
   printf(b->get_name().c_str());
   b = a;
   printf("\n");
   printf(b->get_name().c_str());
   a->set_name("Another test");
   printf("\n");
   printf(a->get_name().c_str());

   delete a;
   printf("\nDeleted a");
   delete b;
   printf("\nDeleted b");

   return 0;
}

If I call a->set_name("Another test");, I get a segfault. If I leave out the call, no problems, everything works. I'm probably missing something simple, but I can't find it. It doesn't segfault on the assignment, but if that line is there it crashes when deleting the pointer.

Answer 1

Since you didn't new anything in the constructor, it is wrong to delete anything in the destructor. Just leave the destructor empty, or even better yet, get rid of it completely. The compiler-generated destructor does exactly what you want (nothing). You also don't have to write the copy constructor and the copy assignment operator by hand, just throw them away.

There is also no need to create the objects dynamically, and printing strings is a lot easier with the native C++ input/output facilities. And you can get rid of overloading the constructor with default arguments. And passing and returning strings by reference-to-const is more efficient than passing them by value. Last but not least, let's be const correct. Here is my cleanup of your code:

#include <iostream>

class object
{
    short id;
    std::string name;
    SDL_Rect offset;

public:

    object(short i = 0, const std::string& n = "");
    const std::string& get_name() const;
    void set_name(const std::string& n);
};

object::object(short i, const std::string& n) : id(i), name(n)
{
    offset.x = 0;
    offset.y = 0;
    offset.w = 0;
    offset.h = 0;
}

const std::string& object::get_name() const
{
    return name;
}

void object::set_name(const std::string& n)
{
    name = n;
}

int main(int argc, char** argv)
{
    object a(0, "test");
    object b(1, "another test");

    std::cout << a.get_name() << "\n";
    std::cout << b.get_name() << "\n";

    b = a;
    std::cout << b.get_name() << "\n";

    a.set_name("Another test");
    std::cout << a.get_name() << "\n";
}

Answer 2

delete &name;

You can only delete pointers that you obtained by calling new. name is a member variable of the class; you can't delete it, nor will you ever have to. When the class is destroyed, it will destroy any member variables as well.

It's only if you have members that are pointers (like if you had std::string* name) that you would have to be sure to properly clean them up, but even then you should prefer to use smart pointers like scoped_ptr, shared_ptr, or unique_ptr (if your implementation supports it).

Answer 3

name is a string and manages its own memory. Your declaration in the struct is correct, you can just assign on top of any value that's already there. No need to do anything when the struct goes out of scope either.

Note that your get_name method returns a new copy since it returns by value. You could instead return a reference to what's in the struct like this, provided the returned value is not used after the owning struct goes out of scope:

const string& get_name() { return name; }

You should change the setter declaration as follows, since there's no benefit in passing a copy (excessive pass by value is a common error among new C++ developers).

void set_name(const string& newValue)