Using Uninitialized Memory

[devious dm]

I've been working on a console game where you play as a fighter, and recently added a "save" feature. Since adding that and slightly changing one of my functions, I've been getting a "using uninitialized memory" warning from the IDE (C6001). When I try to compile, the error list doesn't include that warning, but I'm guessing that warning is causing most of the errors, since none of them make sense and the IDE freaks out when I do what it says it wants (stuff like "Monster;*"). Here's a couple snippets of what's going on.

  Character* Fighter;        // Initialize pointer to new class object "Fighter"

  ... // other code
  
  std::cout << "\nWould you like to load a previous game, or start a new game? (l/n)\t";
  std::cin >> menuchoice;

  ...

        if (menuchoice == 'l') {
            std::ifstream newfile("fighter.sav");
            if (newfile) {
                std::cout << "\n\n\tLoading game...\n";
                newfile.read((char*)&(*Fighter), sizeof(*Fighter));
                std::cout << "\n\tGame Loaded.\n\n";
            }
            else {
                std::cout << "\n\nNo save file detected. Starting new game.\n\n";
                menuchoice = 'n';
            }
        }
        if (menuchoice != 'l') {
            std::cout << "\nWhat kind of Fighter do you want to be?\n";
            /* Player selects type, stored to 'fightertype' */

            Fighter = new Character(fightertype);
        }

        Fighter->viewStats();

Right at "Figher->viewStats();" is where it's saying it's "using uninitialized memory 'Fighter'". I've tried different ways to initialize it, including a second constructor that just sets all the member data for 'Fighter' to 0 or "none" just so there are values there. Which worked before I changed this function (not a class member function):

Monster* random_monster(Character* pfighter); // declaration

Monster* random_monster(Character* pfighter) {  // definition
 	int level = pfighter->getCharLvl();
	int n = rand() % (4 + level);
	if (n < (4 - level)) {
		return new Goblin();
	}
	else if (n < (5 - level / 2)) {
		if ((rand() % 100) < 75) return new Ogre();
		return new GoblinSkeleton;
	}

	/* and so on for other monsters */

        else {
                return new Dracolich
        }
}

All I changed here was adding the parameter so I could use the player's level to determine which monsters they are more likely to encounter, and I added a few more monsters.

[Repeater]

I can see a path through your code such that you never create a Character object, and your pointer never points at it.

Basically, this:

if (menuchoice == 'l') {

If your code enters this path, then no Character object is created, the pointer is left pointing at random memory, and everything is bad.


Character* Fighter; // Initialize pointer to new class object "Fighter"
This doesn't initialise anything. Literally nothing gets initialised here. No class objects are created here, this pointer is pointing at random memory.

[devious dm]

Like I said, it worked before I changed the function.

So, I reinstated the default constructor at the initialization, reworked a few of my #include s, and changed Fighter->viewStats(); to (*Fighter).viewStats();, and I can compile now. In the process, I discovered that the reason it was giving me that warning is because a destructor was being called somewhere in there, likely within one of the if statements, so if I can find and fix that, it'll be golden.

[Repeater]

Like I said, it worked before I changed the function.

If you're creating pointers that point at random memory, and then you use those pointers, your code is wrong. Bad. That it works would be by chance. Writing incorrect code and then hoping that you get lucky and it works by chance is not a sensible programming strategy.

As an aside, in modern C++, new is a bad sign. Raw pointers like in this code are a bad sign.

[devious dm]

Okay, one step at a time...

First of all, I'm very new to this. You're coming across as very rude, just telling me I'm wrong or it's bad without providing much/any context. Part of the problem was that there was an issue with my #include s, which is why some things weren't working. Fixed that. Yes, part of the problem was that the pointer was not actually initialized, just declared. Fixed that with the default constructor.

Second, this code is not working by chance. If you read through it, you'll see that if a save state doesn't exist, it changes 'menuchoice' so that it will enter the next 'if' statement and create 'Fighter' there.

Third, since I'm new to this, I don't know why using new is bad, and I would appreciate an explanation, and maybe a way to get around using it. I used it because I had a similar issue to this before where allocating the object on the stack caused errors (due to destructors) that prevented me from compiling the code. I used new because it allowed me to allocate on the heap, which worked around the error. I haven't yet found a better way to work around that issue.

Thank you for taking the time to look through my code and answer the question. Please provide a little more information about what it is I can improve on.

[Repeater]

Yes, we're very curt around here. Such is life.

     if (newfile) {
                std::cout << "\n\n\tLoading game...\n";
                newfile.read((char*)&(*Fighter), sizeof(*Fighter));

See that line where you use the pointer Fighter and you write some data into some memory? That pointer is pointing at random memory, so you're writing over random memory. This is bad.

I used it because I had a similar issue to this before where allocating the object on the stack caused errors (due to destructors) that prevented me from compiling the code.

While it is possible to write classes that can only exist on the heap, I suspect you haven't done that. So this doesn't make any sense.

I wonder if you actually mean that you are making mistakes controlling the lifetime of your objects? Using new will "solve that", in that the objects will then live forever (or at least until you call delete), but that's exactly the wrong way to fix the problem. You should fix it by designing your code such that objects live as long as you need them to.

new ; it's manual memory management. It's easy to make mistakes. It's easy to cause memory leaks, or to accidentally use pointers that aren't pointing to anything (which is what you're doing). These days, we use smart pointers where we have to, but we prefer to put it on the stack; not on the heap.

[devious dm]

Yes, it was an object lifetime issue. I'll look into smart pointers.

I'm guessing, though, that learning to control object lifetime will come mostly by experience, and trial and error. It'll be difficult for this one, since my code has gotten pretty messy and I'm having a hard time wrapping my head around dealing with object lifetime through several different loops, but I'll be more careful about it in future projects.

Thanks again.