1

I'm new to coding and was just learning stack and the code below is not giving any output. Can you guys please help me out with what I'm doing wrong in this?

#include<iostream>
using namespace std;

struct stack
{
    int size;
    int top;
    int *arr;
};

int isEmpty(struct stack *ptr)
{
    if(ptr->top==-1)
    {
        return 1;
    }
    return 0;
}

int isFull(struct stack *ptr)
{
    if(ptr->top==ptr->size-1)
    {
        return 1;
    }
    return 0;
}

int main()
{
    struct stack *s;
    s->size = 80;
    s->top=-1;
    s->arr= new int(80);
    
    if(isEmpty(s))
    {
        cout<<"the stack is empty"<<endl;
    }
    else
    {
        cout<<"the stack is not empty"<<endl;
    }
}
2
  • In C++ there is no need to write struct stack *ptr, stack *ptr will do the job. Commented Jan 22, 2021 at 6:35
  • 1
    s->arr= new int(80); allocates 1 int and sets it's value to 80. It looks like you want to do s->arr= new int[80];. Commented Jan 22, 2021 at 11:43

2 Answers 2

2

Your main bug is that you haven't initialized s to point to anything. You set s->arr using new, but haven't done so for s itself. You dereference s when setting the size, top, etc, and also when checking top. This is undefined behavior.

You need:

struct stack *s = new struct stack;

to fix this.

Also, always remember to check the allocated pointer(s) are not nullptr before proceeding.

You also don't de-allocate the memory (using delete), so you have a memory leak.

Also, you can use instead the defined true and false, instead of 1 and 0, and change the return types to be bool.

Here is a corrected version of your code with the mentioned changes:

#include <iostream>

struct stack {
    int size;
    int top;
    int *arr;
};

bool isEmpty(struct stack *ptr) {
    if (ptr->top == -1) {
        return true;
    }
    return false;
}

bool isFull(struct stack *ptr) {
    if (ptr->top == ptr->size - 1) {
        return true;
    }
    return false;
}

int main(void) {

    /* you have to initialize s to point to something */
    struct stack *s = new struct stack;
    
    /* check for nullptr */
    if (s == nullptr) {
        return EXIT_FAILURE;
    }
    
    s->size = 80;
    s->top = -1;
    
    /* array of 80 integers */
    s->arr = new int[s->size];
    
    /* check for nullptr */
    if (s->arr == nullptr) {
        return 1;
    }
    
    if (isEmpty(s)) {
        std::cout << "the stack is empty\n";
    } else {
        std::cout << "the stack is not empty\n";
    }
    
    /* remember to de-allocate the memory -- in the reverse order */
    delete[] s->arr;
    delete s;
    
    return 0;
}

Tip: always compile your code with some basic compilation flags to check for this sort of thing. For example, you might compile using at least:

g++ -Wall -Werror -Wextra program.cpp

Also, consider using valgrind, which is a powerful tool for detecting memory leaks and uninitialized memory accesses.

A few extra notes:

  • The code reads like a C program more or less, as pointed out in the comments, so consider whether you want to instead write it as a C program, or adapt it more to C++
  • You could also instead declare s to be on the stack instead (struct stack s;), access its fields in main like s.size, s.top, etc and then pass s in by its address (&s).
Sign up to request clarification or add additional context in comments.

8 Comments

I wouldn't say that assigning a new stack is needed as a fix. It is a fix, but creating the object on the stack (stack s;) is a cleaner fix (because then there is no need to delete s;).
@jamit I feel it makes more sense to keep s in the heap as well, and seemed more in line with what the OP was originally going for. But, if the OP doesn't mind having the "stack on the stack", then that would also work.
Why would it make more sense to keep s in the heap? What benefit is there to offset the costs?
Its true, you could have it on the stack which is perfectly fine. I just thought that since the OP declared it as a pointer initially, they wanted it that way, so stuck with it rather than moving it to the stack. As for benefit of keeping it that way, that would depend on the OP's broader purpose, which I'm not aware of.
I'd definitely upvote such an answer that addresses the broader issues here. After all, that's the beauty of SO, offering various different approaches and explanations, each answer can address the question in a different way and collectively the entire thread has value.
|
0

I have commented out where some changes were to be made...

#include<iostream>
using namespace std;

struct stack
{
    int size;
    int top;
    int* arr;
};

int isEmpty(struct stack* ptr)
{
    if (ptr->top == -1)
    {
        return 1;
    }
    return 0;
}

int isFull(struct stack* ptr)
{
    if (ptr->top == ptr->size - 1)
    {
        return 1;
    }
    return 0;
}

int main()
{
    // s should point to something for initializing top to -1;
    struct stack* s = new struct stack; 
    s->size = 80;
    s->arr = new int(80);
    // Top is initialized to -1 otherwise the output will be shown as The stack is not empty.
    s->top = -1;   
    if (isEmpty(s))
    {
        cout << "the stack is empty" << endl;
    }
    else
    {
        cout << "the stack is not empty" << endl;
    }
}

Comments

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.