0

I'm getting ready for an object oriented class starting in a few weeks, but I'm having trouble with the concept of deallocating memory. RIght now I'm just allocating memory to the first node of the stack, and then trying to deallocate it. If anyone could help me find what I am doing wrong with my destructor, it would be greatly appreciated. Thanks in advance!

The output I am getting from valgrind memcheck is:

==13653== 8 bytes in 1 blocks are still reachable in loss record 1 of 1
==13653==    at 0x4847DA4: operator new(unsigned int) (vg_replace_malloc.c:328)
==13653==    by 0x10A2B: Stack::AddNode(int) (Stack.C:25)
==13653==    by 0x10923: main (Main.C:5)
==13653==
==13653== LEAK SUMMARY:
==13653==    definitely lost: 0 bytes in 0 blocks
==13653==    indirectly lost: 0 bytes in 0 blocks
==13653==      possibly lost: 0 bytes in 0 blocks
==13653==    still reachable: 8 bytes in 1 blocks
==13653==         suppressed: 0 bytes in 0 blocks

Code:

#include <iostream>

using namespace std;

class Stack {
  private:

    struct node{
      int data;
      node * prev;
    };

    node * stackptr;

  public:

   Stack() {
     stackptr = nullptr;
   }

   ~Stack() {
      cout << "Calling destructor" << endl;
      node * p1 = stackptr;
      node * delptr = nullptr;

      while(p1 != nullptr) {
        delptr = p1;
        p1 = p1->prev;
        delptr->prev = nullptr;
        cout << "Deleteing " << delptr->data << " from the stack" << endl;
        delete delptr;
      }
   }

   void AddNode(int data) {
      node * n = new node;
      n->data = data;

      if(stackptr == nullptr) {
        stackptr = n;
        stackptr->prev = nullptr;
      }
      else {
         n->prev = stackptr;
         stackptr = n;
      }
   }
};

int main() {
  Stack s;
  s.AddNode(1);
  s.AddNode(2);
  return 0;
}

EDIT: I have finished my AddNode class. I am still having a similar issue, but with two memory leaks now (The two calls to AddNode in main).

EDIT: Using C++ version 6.3.0 and valgrind version 3.13.0

Employed Russian
  • 199,314
  • 34
  • 295
  • 362
John
  • 33
  • 7
  • 5
    (in general) Don't do manual memory management in modern C++. It's no longer the 90's - we have better ways. Research containers and smart pointers (and make sure you are using a up-to-date version of C++ like C++17 (or *at least* C++14)). Ohh and `using namespace std;` is going to bite you eventually - just ditch that bad habit now. – Jesper Juhl Aug 15 '18 at 17:23
  • 2
    In `Stack::AddNode(int data)` what happens to `n` if `stackptr` is not null? – drescherjm Aug 15 '18 at 17:28
  • 2
    `Stack::Stack() { stackptr = nullptr; }` at least use the constructors initialization list (or in-class initialization) - the constructor body is a last resort that comes with performance implications (object first default constructed, then assigned to, rather than just initialized once). Doesn't matter much for a simple pointer, but for complex objects it can be a killer and may not even be *possible* (and it's just a bad habit in general, with no up sides). – Jesper Juhl Aug 15 '18 at 17:32
  • 1
    @Jesper Juhl I won't be able to use smart pointers in my class, so I figured it would be best to practice using them the way I am. I agree that smart pointers are much better to use though. Thank you for the info! I will have to look more into constructor initialization lists. – John Aug 15 '18 at 17:36
  • @user4581301 and drescherjm, as of right now, nothing happens. I'm only adding one thing to the stack to test it, and then trying to deallocate it. After I figure that out I will add more to AddNode. – John Aug 15 '18 at 17:40
  • 1
    The reason I asked is this is a memory leak. `n` is not added to the stack and is leaked if `stackptr != nullptr`. – drescherjm Aug 15 '18 at 17:46
  • 1
    In that case I don't see a problem. Can we trouble you for a [mcve] so we can recreate exactly what you are doing? Off topic: Here's what I wrote to make sure your destructor was sane: https://ideone.com/KQyTd1 . You may find the `AddNode` method useful. – user4581301 Aug 15 '18 at 17:46
  • @user4581301 I have updated my original post with my minimal, complete example. I did my AddNode function a bit different than you did, but it looks like it is doing the same thing. Thank you for your help by the way. – John Aug 15 '18 at 19:41
  • 1
    Not a problem. I'm not able to reproduce with or without valgrind. The leaks you have are tagged as still reachable. Could be that they are simply not being freed before the report is being generated. Make sure the program output announcing the deletes is before the valgrind output announcing the leaks, not after. – user4581301 Aug 15 '18 at 20:36
  • 1
    Because I forgot that you are making a stack, my addnode routine wouldn't be that useful to you. I wrote it to add nodes to the end. Since all you want to do is push and pop from the same spot, you might as well make it the beginning and you can't write it much better than what you've got unless you add a constructor to `node`. – user4581301 Aug 15 '18 at 20:54
  • @user4581301 I found out what it was! It's kind of odd, but when I removed "#include " and all "cout" statements, I had no more still reachable memory. It must be something with including the library. – John Aug 15 '18 at 20:59
  • 1
    Interesting. If you can dig up the version of the compiler you are using and add it to the question someone might see it and go, "Oh. THAT bug." and let you in on a part of the mystery – user4581301 Aug 15 '18 at 21:38
  • @user4581301 That's a good idea! I have added the version of c++ and valgrind that I'm using. – John Aug 16 '18 at 01:06

0 Answers0