3

I am trying to implement a Stack using a Linked List in C++. When I run my code, nothing is outputted to the console, but it compiles without errors. The problem seems to come from my pointer for the top node. I initially made the top node without a pointer, but that created problems of its own when I tried to initialize it as NULL.

Code:

#include <iostream>
using namespace std;
class Stack{

    class Node{
        int data;
        Node* prev;
        public:
            Node(int x){
                data=x;
            }
            void set_prev(Node nd){
                *prev=nd;
            }
            Node get_prev(){
                return *prev;
            }
            int get_data(){
                return data;
            }
    };

    Node* top = NULL;
    int count = 0;

    public:
        void push(int x){
            Node new_node(x);
            new_node.set_prev(*top);
            *top = new_node;
            count++;
            cout << "Pushing" << endl;
        }
        void pop(){
            if(!is_empty()){
                int data = (*top).get_data();
                *top = (*top).get_prev();
                count--;
                cout << "Popping" << endl;
            }else{
                cout << "Stack is empty." << endl;
            }
            
        }
        int peek(){
            return (*top).get_data();
        }

        int get_count(){
            return count;
        }

        bool is_empty(){
            return !count;
        }

};

int main(){
    Stack stk;
    stk.push(5);
    stk.push(13);
    cout << stk.peek() << endl;
}
zohani
  • 53
  • 5
  • 2
    In set_prev why don't you take in `Node*` and set `prev` to that? Also in `push` `new_node` will be destroyed at the end of the function. You should use dynamic allocation. Also I don't see how you're not getting a segfault at `*prev = nd`, since `prev` is never initialised. Yep tested and a segfault before that dereferencing `top`. The program doesn't output anything because it crashes beforehand – Lala5th Aug 18 '21 at 01:50
  • @Lala5th Regarding the segfault, it should be fine because prev is never accessed before initialization. – zohani Aug 18 '21 at 01:53
  • Well you are dereferencing it before initialisation. `prev` and `*prev` are not the same thing. To initialise `prev` you need `new` `malloc` or some other way to create memory where `Node` can be placed. Also the segfault is definetely a problem given that the linux kernel has this to say: `segmentation fault (core dumped)` at line `new_node.set_prev(*top)` – Lala5th Aug 18 '21 at 01:55
  • *it compiles without errors* simply means that the syntax is correct. It has nothing to do with whether the logic is correct, though. This is a perfect time for you to learn to use a debugger to step through the code, so that you can trace through the execution path and see what's happening and where your logic goes wrong. – Ken White Aug 18 '21 at 01:55
  • @KenWhite I didn't mention it, but I cannot debug this program. When I tried, I get the following error: "C/C++ gcc.exe build active file" terminated with exit code -1. – zohani Aug 18 '21 at 01:57
  • @Lala5th I'm not familiar with the `new` keyword, but I tried initializing new_node as a pointer this way with no luck: `Node* new_node = new Node(x);`. How would you use `new` to avoid the new_node object from being destroyed? – zohani Aug 18 '21 at 02:08
  • @zohani That is the syntax, but a lot of things change due to `new_node` being a pointer. I suggest you look up pointers, and their usage in C++. Also the answer provided below gives a good explanation on why your code fails in general, so it is a good reference point – Lala5th Aug 18 '21 at 02:11
  • 1
    You *cannot* use `new` safely without understanding it first. – Beta Aug 18 '21 at 02:12

1 Answers1

3

There are multiple, fundamental errors in the shown code related to how pointers and objects work in C++. It's not just one issue or error, all of these issues must be fixed before it works correctly.

Node* prev;

This is a pointer member of the Node class. Before using an object referenced by a pointer, the pointer must be set to point to a valid object.

There's nothing in the shown code that appears to set prev to point to any valid Node object.

void set_prev(Node nd){
            *prev=nd;
}

This assigns one object to the object referenced by the prev pointer. The prev pointer has never been initialized to point any object, anywhere. Therefore its value is uninitialized, random garbage. Assigning to an object that's referenced by a pointer which is random, uninitialized garbage is undefined behavior, and a near guaranteed crash.

It is clear, from examining the rest of the code, that the intent here is to pass a pointer to another Node object, rather than a Node object itself; then set the prev pointer to the passed-in pointer value.

        Node new_node(x);
        new_node.set_prev(*top);

So, over here, set_prev() should be called with a pointer to a new_node, rather than passing (a copy of) it to set_prev(). However, the problems are far from over. new_node is an object that's declared in automatic scope. After this function returns, new_node gets destroyed. Any existing pointer to it now points to a destroyed, no-longer valid object, and dereferencing it further results in undefined behavior, and another, very likely crash.

It is clear, based on the context, that the intent here is to instantiate a new Node object in dynamic scope, using the new keyword. Consequently, pop() is expected to delete them, as well.

This kind of a homework assignment is traditionally given after introducing the concept of dynamic scope, and using new and delete to create objects in dynamic scope. You should review your class notes, or textbook material, for more information on this topic, and additional details on how to properly, and correctly, create and destroy objects; and proper use of pointers.

Sam Varshavchik
  • 114,536
  • 5
  • 94
  • 148
  • Actually, I never learned C++ in school, just thought I'd try to code a stack to learn something. Thanks for the very detailed answer! – zohani Aug 18 '21 at 02:50
  • 1
    C++ is the most complicated and hardest to learn general purpose programming language in use today. Don't waste your time on web sites or Youtube videos. Any clown can create a web site for their streams of consciousness, or upload a rambling video to Youtube. You won't learn C++ from those. If you'd like to continue learning [see Stackoverflow's list of C++ textbooks](https://stackoverflow.com/questions/388242/). – Sam Varshavchik Aug 18 '21 at 11:06