0

I am fairly new to C++ and I keep getting segmentation fault with pointer assignment with code similar to the following, I know it means I'm accessing memory that hasn't been allocated, but I don't see where:

I have two classes:

class ClassA{ //class a decl.
  ClassB** oArray;
  unsigned int x;
 public:
  ClassA(unsigned int X);
  void oMember(ClassB* classb);
}


ClassA::ClassA(unsigned int X){ //Constructor for class a
  x = X;
  oArray = new ClassB* [x];

 for (unsigned int i = 0; i < x; i++){
        oArray = NULL; 
    }
}

class ClassB{ //rough decl of class B
  public:
   getId();
}

I have a class member function that takes in a pointer to another class like this:

void ClassA::oMember(ClassB* classb){
   unsigned int cID = classb.getId(); //defined in class b
   oArray[cID] = classb; //if cID is less than x defined in constructor, is this legal?
}

I just want to point the cIDth member of the array to classb.

I keep getting segmentation fault with an assignment similar to the above. I don't quite know why, I printed the cID and it is definitely less than the size of the array we declared in ClassA's constructor.

Why is that assignment illegal, or why am I getting a segmentation fault?

halfer
  • 19,824
  • 17
  • 99
  • 186
anpatel
  • 1,952
  • 4
  • 19
  • 36
  • 1
    Something you should follow since your class is allocating memory: [Rule of Three](http://stackoverflow.com/questions/4172722/what-is-the-rule-of-three). On another note, `ClassB *classb) { ... classb.getId();` shouldn't compile. – chris Jul 22 '12 at 16:27
  • You would make this easier if you would post code that helps us reproduce the problem (The code hear wont compile). Otherwise there are so many things wrong with the code, that it is unlikely that we can really help you. – pmr Jul 22 '12 at 16:29
  • @pmr, alright I'll post the code – anpatel Jul 22 '12 at 16:32
  • As chris implied in his comment, this line `unsigned int cID = classb.getId();` should probably instead be `unsigned int cID = classb->getId();`. – Turix Jul 22 '12 at 16:32
  • @Turix yeah it was a typo in translating my code to a simpler version.. i've put up the code I'm actually working with – anpatel Jul 22 '12 at 16:36
  • @pmr I'm working with a lot of dependencies.. but rest assured the code does compile. It prints proper out put as well for a bit then gives me segmentation fault when it goes to print the else case of the VMRegister if statement – anpatel Jul 22 '12 at 16:38
  • Why do you call "DeleteVMList(RegisteredVMs, nVendingMachines);" twice in the same if block without any modification to RegisteredVMs? I assume DeleteVMList would try to delete something from the first argument – PermanentGuest Jul 22 '12 at 16:46
  • @PermanentGuest I've called it once to delete the members of RegisteredVMs array and the second time to delete the members cRegisteredVMs. If you look closely they are copies, I needed to allocate more to the register array. After I've created a new array with the same elements I've freed the memory used by the prev. ones..I've tested the > than case, and it works perfectly fine its the else case that leaves me with seg. fault – anpatel Jul 22 '12 at 16:47
  • @MyName: between 4th and 10th lines, the variable RegisteredVMs is not modified. And moreover you use RegisteredVMs[i]; in the 7/8th line after deleting in the 4th line. – PermanentGuest Jul 22 '12 at 16:55
  • @PermanentGuest I SEE it. Yes you're right the first one is an extra call to delete – anpatel Jul 22 '12 at 16:57
  • @PermanentGuest fixed that :D but i still get seg. fault.. I get it for cases where vID is < nVendingMachines – anpatel Jul 22 '12 at 16:59
  • 1
    @MyName: You shouldn't use pointers. I'm not sure what is the intention of the double-pointers in the class. In the constructor line 9 you allocate the RegisteredVMs pointer. In the immediate next loop, you delete that pointer multiple times.. Why don't you simply use a vector or map and avoid pointers altogether? – PermanentGuest Jul 22 '12 at 17:05
  • @PermanentGuest its a specification I have to abide by for the sake of the exercise. – anpatel Jul 22 '12 at 17:07
  • @MyName: OK, fine. If you are learning pointers, it is OK. In any case, you wouldn't need a double pointer to store an array of machines. Single pointer can be used as an array, if you specify the array size in new – PermanentGuest Jul 22 '12 at 17:10
  • @PermanentGuest I want another member.. getMachineList() to return a an array of pointers to vendingmachines. I think I would need a double pointer to do that. – anpatel Jul 22 '12 at 17:14
  • I understand your confusion. If you want to return the list of machines, then you could simply return the single pointer with the following signature "void getMachineList(VendingMachine*& aMachineList, int& aNumMachines);". Your signature is wrong. When a function returns an array, it should always give the array's size also. Otherwise how the caller would know how many elements are there in the array? In the above signature, you just need a single pointer and this is returned as a reference. – PermanentGuest Jul 22 '12 at 17:18
  • @PermanentGuest this program is 7 class program, with a few constants the upper bound of the array is a constant that all of the classes have access to. See I thought it wasn't which is why I was doing all of that delete stuff, but after you asked me why I looked into it and the numVendingMachies stays constant. Which is why I dont need to return a #. I need to do a double pointer because it is what is required in the exercise... I have no control over the public members of the classes, only the private members – anpatel Jul 22 '12 at 17:22
  • 1
    let us [continue this discussion in chat](http://chat.stackoverflow.com/rooms/14248/discussion-between-permanentguest-and-myname) – PermanentGuest Jul 22 '12 at 17:24

3 Answers3

1

I think you should change your ClassA to

class ClassA{
  std::map<int, std::shared_ptr<ClassB> > mMap;

 public:
  ClassA();
};

Now class A doesn't need to know the array size and mMap will ensure that you don't have very sparse arrays.

parapura rajkumar
  • 24,045
  • 1
  • 55
  • 85
1

As we realized in the chat, I just record the problem here for the reference.

The problem lies in the piece

RegisteredVMs = new VendingMachine *[nVendingMachines];
for (unsigned int i = 0; i < nVendingMachines; i++){
   RegisteredVMs = NULL; 
}

RegisteredVMs is allocated and immediately set to NULL. This pointer is accessed later in the VMregister() function which causes the seg. fault.

Pointers are hard and very error-prone. Use them only if there is no other way. Since this is a homework problem and since you says you have no say on the interface, I see that you have to use them.

Community
  • 1
  • 1
PermanentGuest
  • 5,213
  • 2
  • 27
  • 36
0

I think classb.getId() should be classb->getId() instead as long as classb is a pointer. Well, that should be compiler error and I don't think it is the reason you get segmentation faults.

Are you sure you instantiated ClassA with that particular constructor? If not, the x and oArray may not be initialised.

I do not have your code. But as I modified your code segment to be following, I find no segmentation faults nor compiler warnings.

class ClassB {
public:
    int getId();
};

class ClassA {
    ClassB** oArray;
    unsigned int x;
public:
    ClassA(unsigned int X);
    void oMember(ClassB* classb);
};

int ClassB::getId() {
    return 0;
}

ClassA::ClassA(unsigned int X) {
    x = X;
    oArray = new ClassB* [x];
}

void ClassA::oMember(ClassB* classb) {
    unsigned int cID = classb->getId();
    oArray[cID] = classb;
}

int main(int argc, char** argv) {
    ClassA a(12);
    ClassB b;
    a.oMember(&b);
    return 0;
}
Kinson Chan
  • 131
  • 1
  • 4
  • Oops. The problem description is significantly changed. I will leave this answer as is here. – Kinson Chan Jul 22 '12 at 16:47
  • Thank you so much. Yeah i had a simpler version, but actual code was requested so i changed it. What you've outlined does help me. I'll have a look at my main function to see if I call the functions properly. – anpatel Jul 22 '12 at 16:52
  • your code does work.. maybe its another class causing the problem. I still get seg fault. – anpatel Jul 22 '12 at 17:01