Accessing member of a C++ object allocated with malloc?

Question

[Editor's note: I have edited the title to try to make this useful to others in the future. To give the answerers credit, this was just a "why does this not work?" question when they answered it!]

The following code crashes at the top -> ... line with a segmentation fault, regardless of what Node* is trying to be pushed onto the vector children. Does anyone know what might be causing this?

struct Node
{
    string data;
    struct Node* parent;
    vector<struct Node*> children;
};

struct Node* push(string word, struct Node* top)
{
    Node* temp = (struct Node*) malloc(sizeof(struct Node));
    temp -> data = word;
    temp -> parent = top;
    return temp;
}

int main()
{
    Node* top = push("blah", NULL);
    Node* temp = NULL;
    top -> children.push_back(temp);
}

Answer 1

The problem is that malloc will not call constructors. And you're counting on the vector children to be constructed.

Replace:

Node* temp = (struct Node*) malloc(sizeof(struct Node));

With:

Node* temp = new Node;

malloc (from C) and new (from C++) will both allocate the memory you need, but only new will call the required constructors, as C doesn't use them. If you're not positive that you need malloc, use new.

Answer 2

You don't use malloc on objects, you should use new instead. malloc is a C function that just allocates a chunk of memory, while the C++ operator std::new also takes care of the object initialization and construction - step that you've missed here and that causes you troubles (for example the constructor for temp->children has never been called in your case).

As a rule of thumb: if you're writing C++ code you should use the C++ operators std::new and std::delete for dynamic memory allocations and releases, not the C functions.

Answer 3

Your problem is that your children vector isn't correctly initialized. You should use Node* temp = new Node; instead of malloc to call the constructor for Node, which calls the constructor of children, thus correctly initializing the vector

Answer 4

As others have commented, it looks like you have come from C and are in need of a good C++ book. C++ is not just "C with classes"!

Your push function looks very much like it should be a constructor. A constructor is called by new after it has allocated the required memory and performs the necessary initialisation. The compiler will generate one for you, if you don't provide one (it will also provide a copy constructor and an assignment operator (see What is The Rule of Three?).

Since you called malloc() and not new, the synthesised default constructor was not called and therefore the children vector was not initialised, which caused your access violation.

Here I demonstrate how you may implement a default constructor (and disable the other two), to initiate each of the three data members of your class (or struct):

#include <string>
#include <vector>
using std::vector;
using std::string;

class Node
{
public:
    Node(const string& word, Node* top)
        :data(word)
        ,parent(top)
        ,children()
    {
        if (top != NULL)
        {
            top->children.push_back(this);
        }
    }

    virtual ~Node()
    {
        std::vector<Node*>::iterator i = std::find(children.begin(), children.end(), this);
        if (i != children.end())
        {
            parent->children.erase(i);
        }
        // TODO: You may wish to destory children at this point
    }

private:
    Node(const Node& disable);
    Node& operator =(const Node& disable);

private:
    string data;
    Node* parent;
    vector<Node*> children;
};

int main()
{
    Node top("blah", NULL);
    Node child("foo", &top);
}

I also implemented a destructor, which removes a node from its parent's children on destruction.

Answer 5

malloc() just allocates an empty block of memory, you should use the new() operator which initializes all member objects;

Node* temp = new Node();