Pointers, polymorphism and segmentation fault in C++

Question

I have a Parent class with a number of children. Each function in the Parent class is pure i.e. there are no parent implementations of the functions but the children have their own implementations. No need to post code there - standard stuff.

I don't want people creating direct instantiations of the parent class from anywhere. I have safe-guarded against this by having the virtual functions being pure so that's fine.

My problem: Depending on a user's input (a string), I want to instantiate one of the children. I only know which one at runtime. My (incorrect?) idea was the following which compiles fine and works fine until I put the code into a function and return the parent.

So this works:

Parent* parent;
if(user_input == "A") {
    Child1 child1;
    parent = &child1;
}
else if(user_input == "B") {
    Child2 child2;
    parent = &child2;
}

but this does not work:

Parent* foo(string user_input) {
    Parent* parent;
    if(user_input == "A") {
        Child1 child1;
        parent = &child1;
    }
    else if(user_input == "B") {
        Child2 child2;
        parent = &child2;
    }
   return parent;
}

When I say it doesn't work I mean, it compiles but then I get a segmentation error when I do this:

Parent* parent = foo(user_input);
parent->some_child_function(); // segmentation error here

I'm sure it's a stupid/simple question about me not fully understanding pointers??? Well after reading all about them in books/articles, I still don't know what I'm doing wrong.... It's probably a one-line fix?

Thank you :).

score 11 · Accepted Answer · answered Jul 05 '11 at 12:26

11

You run into undefined behavior:

Parent* parent;
if(user_input == "A") {
    Child1 child1; // stack-allocated object
    parent = &child1;
} //child1 is destroyed at this point, the pointer is dangling now

you have to use new (preferably with a smart pointer):

Parent* parent = 0;
if(user_input == "A") {
   parent = new Child1();
} else if(user_input == "B") {
   parent = new Child2();
}

and it's also a good idea to set the pointer to null at the beginning so that you don't have a pointer with a garbage value in case you have an error in your code and the pointer is left uninitialized.

answered Jul 05 '11 at 12:26

sharptooth

167,383
100
513
979

You know you don't need those `()`s with the `new` calls, right? – Nikolai Fetissov Jul 05 '11 at 12:50
@Nikolai - according to [this](http://stackoverflow.com/questions/620137/do-the-parentheses-after-the-type-name-make-a-difference-with-new/620402#620402) and [this](http://ideone.com/exREd), yes, you do need those `()`s with the new calls. – Robᵩ Jul 05 '11 at 17:33
Hmm, no. Proper default initialization is the responsibility of the class, not the caller. Or so I think ... – Nikolai Fetissov Jul 05 '11 at 17:58

score 5 · Answer 2 · answered Jul 05 '11 at 12:30

try this

Parent* foo(string user_input) {
    Parent* parent;
    if(user_input == "A") {
        Child1 *child1 = new Child1();
        parent = child1;
    }
    else if(user_input == "B") {
        Child2 *child2 = new Child2();
        parent = child2;
    }
   return parent;
}

Basically in your version the object was created on the stack, and by the time your method returned it gets purged. using new ensures the object is created on the heap, so it remains there until deleted.

But callers of your method need to make sure they delete the objects otherwise you have memory leaks.

For production code using some library like boost is more appropriate.

score 4 · Answer 3 · answered Jul 05 '11 at 12:39

Slightly shorter version of some of the other examples... get rid of the Parent pointer altogether, and allocate your variable using new. Don't forget to destroy it later using delete.

Parent* foo(string user_input)
{
    if(user_input == "A") {
        return new Child1();
    }
    else if(user_input == "B") {
        return new Child2();
    }
   return NULL;
}

score 3 · Answer 4 · answered Jul 05 '11 at 12:29

3

You have to allocate instances of the implementation classes on the heap with operator new. You also want to look into Factory Method Pattern.

answered Jul 05 '11 at 12:29

Nikolai Fetissov

82,306
11
110
171

score 1 · Answer 5 · answered Jul 05 '11 at 12:27

1

In your functions parent is a pointer to a local object, which is destroyed as soon as the program leaves the function scope. Try constructing object in heap, like parent = new child1; and don't forget to delete parent after use.

answered Jul 05 '11 at 12:27

Grigor Gevorgyan

6,753
4
35
64

score 1 · Answer 6 · answered Jul 05 '11 at 18:27

You should look into the Factory pattern for this solution. Using this pattern will allow any users creating different parent objects to only worry about the factory's interface. And it will also allow you to add different types more easily down the road. Using this pattern may seem like it's an additional extra step, but it adds a nice layer of abstraction to your project and a little more flexibility for adding things later on.

class ParentFactory
{
    public:
          Parent* foo(const std::string &user_input) 
          {
              if(user_input == "A")
                  return new Child1;
              if(user_input == "B")
                  return new Child2;
              return 0;
          }
};

Pointers, polymorphism and segmentation fault in C++

6 Answers6