C++ Program execution halts with no error

Question

I've started to experiment (no previous experience with the language) with c++, I'm using Dev C++ ver. 5.11. I'm trying to implement a stack, I don't want to use the ready Stack library because I want to practice. However with no errors the program halts after printing out the stack items and further statements aren't executed. What is going wrong?

#include <iostream>
#include<stdio.h>
#include<stdlib.h>

using namespace std;

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

struct node *push(struct node *root, int ikey){
    struct node *ptr;
    
    ptr=(struct node *)malloc(sizeof(struct node));
    ptr->data=ikey;
    ptr->next=root;
    root = ptr;
    return root;
}
struct node *pop(struct node *root){
    struct node *ptr;
    
    ptr = root;
    root = ptr->next;
    return root;
}
void Print_Stack(struct node *root){
    struct node *n;
    n = root;
    do{
        printf("Node -> %d \n",n->data);
        n = n->next;
    }while (n != NULL);

}

int main(){
    struct node *stoiva, *tmp;
    int x;
    tmp = (struct node *)malloc(sizeof(struct node));
    do{
        printf("Give x :");
        scanf("%d",&x);
        if (x > 0){
            stoiva = push(stoiva, x);
        }
    }while ( x > 0);
    tmp = stoiva;
    do{
        cout << tmp->data << endl;
        tmp = tmp->next;
    }while (tmp->next != NULL);
    printf ("-----------------\n");
    return 0;

}

In `main` the variable `stoiva` is uninitialized. Imagine what that does for the end of your list. Also, you're leaking memory; `tmp` in `main` is near pointless. And, you should be using `new` and `delete` (and ideally, smart pointers and/or `std::list`, but I realize this is academic). — WhozCraig, May 23 '23 at 08:13
Your style is very C-like, so you are really practicing C, not so much C++. Nothing wrong with that, of course. — nielsen, May 23 '23 at 08:16
If you really want to practice, then practice C++, don't write C code. Forget `malloc` and `NULL`, and `#include` and `#include`. — 273K, May 23 '23 at 08:17
this doesnt look like c++. In c++ you do not need to deal with manual memory managment via raw pointer, `malloc` is almost always wrong, the headers are `cstdio` and `cstdlib`, `using namespace std;` is totally unnecessary in this code, `NULL` should not be used anymore, `struct node *stoiva, *tmp;` is a c-ism in c++ you can write `node* stoiva; node* tmp;`, and there is more.... better use a [book](https://stackoverflow.com/questions/388242/the-definitive-c-book-guide-and-list) to learn c++ — 463035818_is_not_an_ai, May 23 '23 at 08:17
Look for the place where you set the `next` pointer of the last node to the null pointer. (Related: don't leave your variables uninitialized.) — molbdnilo, May 23 '23 at 08:19
fwiw, using the standard library does require practice too. Its your choice to learn about writing your own datastructure or learn about writing c++ code. What you are doing now it not practicing c++. "I am not using the standard library because I want to practice" is a misunderstanding to some extend — 463035818_is_not_an_ai, May 23 '23 at 08:19
Please don't add "solved" to your question title or body. See [what should I do when someone answers](https://stackoverflow.com/help/someone-answers) on how to show you've solved your problem. TL;DR accept an answer, which you've already done. That suffices to show that this question is solved. — Adriaan, May 23 '23 at 14:20
Apologies I was meaning not use the "Stack" Library provided by C++. — Nik D, Jun 02 '23 at 05:29

Alan Birtles · Accepted Answer · 2023-05-23T08:44:05.523

Your code has a number of issues:

    do{
        cout << tmp->data << endl;
        tmp = tmp->next;
    }while (tmp->next != NULL);

will crash if tmp is initially null (an empty stack) and will also fail to print the last element in the stack. Both problems can be solved with:

    while (tmp){
        cout << tmp->data << endl;
        tmp = tmp->next;
    }

stoiva is not initialised so may not be null, you should always initialise pointers for safety.

You should use new rather than malloc in c++, not only is it required for a lot of c++ types its much simpler too:

ptr=(struct node *)malloc(sizeof(struct node))

becomes:

ptr=new node();

You should check the result of scanf to make sure it succeeded before using the resulting value.

You need to free the memory you allocated, in your case you need to use free but if using new you need to use delete.

A fully working version would be:

#include <iostream>
#include<stdio.h>
#include<stdlib.h>

using namespace std;

struct node{
    int data;
    node *next = nullptr;
};

node *push(node *root, int ikey){
    node *ptr = new node();
    ptr->data = ikey;
    ptr->next=root;
    root = ptr;
    return root;
}

node *pop(node *root){
    node *ptr = root;
    root = ptr->next;
    delete ptr;
    return root;
}

int main(){
    node *stoiva = nullptr;
    int x;
    do{
        printf("Give x :\n");
        if (scanf("%d",&x) != 1)
        {
            break;
        }
        if (x > 0){
            stoiva = push(stoiva, x);
        }
    }while ( x > 0);
    node * tmp = stoiva;
    while (tmp){
        cout << tmp->data << endl;
        tmp = tmp->next;
    }
    while (stoiva){
        stoiva = pop(stoiva);
    }
    printf ("-----------------\n");
    return 0;

}

Thank you very much :) Your input was really useful and much appreciated. Now I understand better C++ :) — Nik D, Jun 02 '23 at 05:29
@Nik D I'd like to argue that now you understand `C` better. Unfortunately there's tons of material online that teaches a very bad or at least antiquated style of C++. If you really want to learn C++ and not C, I'd strongly advise to recreate your stack code in modern C++. — infinitezero, Jun 02 '23 at 07:21
I had had to write code in C for more than 20 years. I rediscovered it recently so from 1 of 10 I understood the language I went to 2 of 10. So for me it's a help, of course there is always more to learn about programming :) — Nik D, Jun 05 '23 at 16:14

C++ Program execution halts with no error

1 Answers1