CODEWITHSUNDEEP
c++vector
DragonVet
DragonVet

Reputation: 409

Unable to populate vector inside struct

I'm new to C++ so bear with me.

I made a struct that looks like this:

struct node{
   double startPoint;
   double endPoint;
   vector<node*> children;

   void addChild(node *aNode){
      children.push_back(aNode);
   }
   void addPoints(double start, double end){
      startPoint = start;
      endPoint = end;
   }
};

Down the line in my program, I have the following:

vector<node*> data;
....
node *temp = (node*)malloc(sizeof(node));
temp->addPoints(lexical_cast<double>(numbers[0]), lexical_cast<double>(numbers[1]));
data[index]->addChild(temp);

where "Index" is a index of the vector data. the lexical_cast stuff is taking those numbers from string to doubles.

Everything works until the addChild(temp) line.

The terminal spit this out:

First-chance exception at 0x585b31ea (msvcr90d.dll) in Tree.exe: 0xC0000005: Access violation reading location 0xcdcdcdc1.
Unhandled exception at 0x585b31ea (msvcr90d.dll) in Tree.exe: 0xC0000005: Access violation reading location 0xcdcdcdc1.

But I have no idea how to deal with that.

Upvotes: 4

Views: 2523

Answers (3)

aaronman
aaronman

Reputation: 18750

  1. I would recommend that you store a node instead of a node* in your vector so you don't have to manage the memory on your own.

  2. this is C++ so you don't have to malloc the space for a node you can use new like so:
    Node * n = new Node();

  3. New is much better because it calls the constructor and allocates space, whereas malloc just does the latter.

You haven't shown much of your code, but I would restructure the node class like this. 

struct node{
   double startPoint;
   double endPoint;
   vector<node> children;
   node(){} //add default constrcutor    
   void addChild(node aNode){
      children.push_back(aNode);
   }
   node & operator=(const node & n) {
       startPoint = n.startPoint;
       endPoint = n.endPoint;
       return *this;
   }
   node(double start, double end): startPoint(start),endPoint(end){
   } //in c++ you have constructors which this should have been in the first place
     //constructors are used for initializing objects
};
  1. this is better is that now you can't pass add child nullptr avoiding a lot of problems in your code. You also have a constructor now. Now you can add a node like this.

node temp(start,end); data[index]=temp;

  1. You have a constructor now which addPoints should have been in the first place
  2. I also made an assignment operator

Using the style of coding where you allocate memory on the stack and don't use new is called RAII and is a vital technique for learning c++ and producing exception safe code, this is the main reason I advocate not storing node*'s

Upvotes: 0

Useless
Useless

Reputation: 67723

malloc allocates some space, but doesn't put anything in it. It works fine for plain-old-data structures (or trivially initializable classes), and in C that's all you have.

In C++ you have classes, like std::vector amongst others, which need to be properly constructed in order to establish some invariants. This is done with a straight declaration for objects with automatic storage duration, but for dynamically-allocated objects you need to use new instead of malloc.

For example,

std::vector<int> global;    // (1)
void foo() {
    std::vector<int> local; // (2)
    std::vector<int> *bad = malloc(sizeof(*bad));  // (3)
    std::vector<int> *good = new std::vector<int>; // (4)
    std::unique_ptr<std::vector<int>> better(new std::vector<int>); (5)
}
  1. is fine - this global is initialized (by which I mean the constructor is called) automatically
  2. is fine - this local variable is also constructed automatically, and destroyed properly as soon as foo exits
  3. you can't use bad for anything, because any method you call will assume the constructor ran already, and it didn't
    • ok, you can't use bad for anything without explicitly constructing it using placement new. You shouldn't do this though, it's only appropriate where you're doing clever or tricky stuff with custom allocation.
  4. this is ok (but note you have to delete it manually - foo has a memory leak)
  5. this is better - you don't need to clean up manually

Now, note that your node class also has a constructor. In this case, it's automatically-generated, and does nothing but call the vector constructor. Still, you need it to be called, which means using new for dynamically allocating a node.

So, your program should probably look more like:

std::vector<std::unique_ptr<node>> data;
...
std::unique_pre<node> temp(new node);
temp->addPoints(...);
data[index]->addChild(temp);

Note I'm assuming data[index] is valid (I see from addChild you know how to populate a vector already), and that the single-owner model implemented by unique_ptr is appropriate.

Upvotes: 2

Lochemage
Lochemage

Reputation: 3974

As far as the code I see, you never add any nodes into the data array

data.push_back(something);

So accessing data[index] would be out of the allocated memory of the array. It won't complain until you try to set memory in that block (via addChild trying to push an element into the children array).

Upvotes: 1

Related Questions