Jump to content
  • Advertisement
Sign in to follow this  
NUCLEAR RABBIT

C++ VirtualFunctions [SOLVED]

This topic is 1003 days old which is more than the 365 day threshold we allow for new replies. Please post a new topic.

If you intended to correct an error in the post then please contact us.

Recommended Posts

Hello,

 

I am messing around with inheritance and I was doing a simple example using it, but for some reason it is not working like I expected it to. When I run the code, the greet() method from the Animal base class is being called on both objects, even though I have it as a virtual function. I thought this would allow the child class to behave differently when used with pointers. Could anyone please help me see what I am doing incorrectly or maybe misunderstood?

/////////////////////////////////
//
//   Random C++ tests on
//   concepts and ideas
//
/////////////////////////////////

#include <iostream>
#include <string>
#include <iterator>
#include <vector>

using namespace std;

class Animal
{
public:
	Animal(string tmp_name)
	{
		name = tmp_name;
	}

	virtual void greet()
	{
		cout << "I am " << name << ", and I am Animal" << endl;
	}
protected:
	string name;
};

class Bird : public Animal 
{
public:
	Bird(string tmp_name) : Animal(tmp_name) {};

	void greet()
	{
		cout << "I am " << name << " and I am a Bird" << endl;
	}
};

//////////////////////////////////////////////////////////////////
//   MAIN
//////////////////////////////////////////////////////////////////

int main()
{
	Animal * steve = new Bird("Steve");
	Animal * david = new Animal("David");

	vector<Animal> animal_list = { *steve, *david };

	for (vector<Animal>::iterator itr = animal_list.begin(); itr != animal_list.end(); itr++)
	{
		itr->greet();
	}

	cin.get();

	return 0;
}

Share this post


Link to post
Share on other sites
Advertisement

Change your Animal vector to an Animal* vector and remove the dereferencing operator from steve and david. The way you have it now, your storing the values of steve and david, not the addresses like you should be.

Share this post


Link to post
Share on other sites

 

Also of note is you are leaking memory, because you newed both a bird and animal, but you never deleted them

Also, it's a good practice to set them to null upon deletion. If you accidentally call delete on a pointer again that isn't set to null, it results in undefined behavior meaning your program could potentially crash.

Share this post


Link to post
Share on other sites

Thanks a lot guys for the help! smile.png

 

Just one last question, does make_unique also auto delete its contents?


// unique_ptr will automatically delete its contents when it goes out of scope

auto steve = std::unique_ptr{ std::make_unique("Steve") };
auto david = std::make_unique("David");

Share this post


Link to post
Share on other sites

Thanks for all your pointers! (no pun intended) Really informative

 

Your vector is storing animal objects by value. This means you aren't storing any birds, simply "slicing" your bird into an animal (via the compiler-provided default copy constructor), so your virtual call isn't being overridden.

If your vector stores pointers, then you'll just be copying the pointer value instead of the object, slicing won't occur, and the virtual call will work as expected.

Also of note is you are leaking memory, because you newed both a bird and animal, but you never deleted them. You should also consider taking your string value by const reference, instead of by value to avoid additional potential copies. Also, since greet() does not alter the state of the object, it should be declared const, and since you are not modifying the objects in your for loop, you should prefer const_iterator. Finally, prefer to keep your class data private and provide a public interface to the data (usually via const accessor) instead of making it protected.

Fixed code (no changes other than fixing vector type and properly deleting):

Animal* steve = new Bird("Steve");
Animal* david = new Animal("David");

vector<Animal*> animal_list = { steve, david };

for (vector<Animal*>::iterator itr = animal_list.begin(); itr != animal_list.end(); itr++)
{
  (*itr)->greet();
}

cin.get();

delete steve;
delete david;
If you are using a C++11/14 compatable compiler (and it seems like you are since you do array initialization of the vector), you should also consider using unique_ptr for your owning pointers, and a ranged-for for your iteration, as follows:
 
// "auto" tells the compiler to pick the appropriate type. In this case, it will pick
// std::unique_ptr<Animal> because that's the type we want. We have to specify this
// for steve because otherwise steve will be of type unique_ptr<Bird>.

// Whether you prefer to use auto or not is up to you, but it does force you to
// initialize your variables.

// make_unique is recommended to avoid corner case issues with new and smart pointers
// unique_ptr will automatically delete its contents when it goes out of scope

auto steve = std::unique_ptr<Animal>{ std::make_unique<Bird>("Steve") };
auto david = std::make_unique<Animal>("David");

auto animal_list = std::vector<Animal*>{ steve.get(), david.get() };

// This loop will iterate over every element in the vector, assigning each to curAnimal
// It's not only easier to type, but more succint, and likely easier for the compiler to
// optimize as opposed to the manual iteration above.

// "auto" here in the ranged-for loop will deduce to the "Animal*" type, the type of
// the vector's elements.

for (auto curAnimal : animal_list)
{
  curAnimal->greet();
}

cin.get();

Share this post


Link to post
Share on other sites

thanks for the help!

 

Change your Animal vector to an Animal* vector and remove the dereferencing operator from steve and david. The way you have it now, your storing the values of steve and david, not the addresses like you should be.

Share this post


Link to post
Share on other sites

Also, it's a good practice to set them to null upon deletion. If you accidentally call delete on a pointer again that isn't set to null, it results in undefined behavior meaning your program could potentially crash.

 

I would like to disagree. If you call delete a second time, you are likely to get an immediate crash on that line, or some sort of memory corruption. You might think this is a bad thing but actually it can be a good thing. Every time something is deleted twice, there is some sort of logic error in the code. Its better to expose those issues and fix them, than just carry them around forever and silently ignore them.

 

So I would never set a deleted pointer to nullptr. If you happen to get memory corruption errors which can be annoying to locate, there is tools like "ApplicationVerifyier" which will track the issue down to the exact line. See why you are deleting the object twice, and fix it. Or better yet, use smart pointers (std::unique_ptr, std::shared_ptr) instead of of manual memory allocation. std::unique_ptr has zero-overhead over a regular pointer but takes care of freeing the object, so you won't have eigther memory leaks nor double deletes anymore.

 

Also, please don't change the threads title to "SOLVED", this could potentially discourage discussions like this.

Edited by Juliean

Share this post


Link to post
Share on other sites


Just one last question, does make_unique also auto delete its contents?

 

make_unique is just a helper introduced in C++14 to create an std::unique_ptr in an exception safe way.

 

So make_unique does not auto-delete anything, but the std::unique_ptr it creates will take ownership of the object, and delete it when the std::unique_ptr goes out of scope.

Share this post


Link to post
Share on other sites

I would like to disagree. If you call delete a second time, you are likely to get an immediate crash on that line, or some sort of memory corruption. You might think this is a bad thing but actually it can be a good thing. Every time something is deleted twice, there is some sort of logic error in the code. Its better to expose those issues and fix them, than just carry them around forever and silently ignore them.

 

I definitely agree with "fail fast", but I disagree with your advice in this particular case. The problem is, passing an invalid pointer to delete doesn't always cause a crash right away (unless you're always running with app verifier, which isn't realistic). It can simply corrupt the heap causing problems later, or in fact it could cause no problems at all if something has already been reallocated at that memory location (not unlikely when space is requested shortly after the delete for a same size object, since there will be a nice hole in the heap there).

 

It also makes it harder to diagnose issues, because you have no idea if the pointer is "valid" when inspecting it in the debugger.

 

The other thing is that it is not uncommon to have code that, say, resets the state of an object, which would include deleting any members that were new'd. In that case, there is no logic error, and you would need to store additional state in order to know if a pointer is valid or not. So why not just set it to null.

 

But yes, definitely use smart pointers if you can (which do set their internal pointer to null when they delete it).

Edited by phil_t

Share this post


Link to post
Share on other sites
Sign in to follow this  

  • Advertisement
×

Important Information

By using GameDev.net, you agree to our community Guidelines, Terms of Use, and Privacy Policy.

We are the game development community.

Whether you are an indie, hobbyist, AAA developer, or just trying to learn, GameDev.net is the place for you to learn, share, and connect with the games industry. Learn more About Us or sign up!

Sign me up!