Initialised arrays showing different values in a loop

Question

I initialised an array and tried displaying the elements using loop and recursion but every time it shows different values than the original ones. I tried displaying the elements individually and it works fine.

This is the class definition in which the array is defined:

class stack
{
    public:
        int top, *arr;
        stack(int s)
        {
            top=-1;
            size=s;
            arr=def_arr(s);
        }
        void push(int num)
        {
            if(top>=size-1)
            {
                cout<<"Stack has reached maximum length";
            }
            else
            {
                top++;
                arr[top]=num;
            }
        }
        int pop()
        {
            if(top>-1)
            {
                int temp;
                temp=arr[top];
                top--;
                return temp;
            } 
            else
            {
                cout<<"The stack has no values";
            }
        }
        void print()
        {
            if(top>-1)
            {
                for(int i=0; i<=top; i++)
                {
                    cout<<arr[i];
                    cout<<"\t";
                }
            }
            else
            {
                cout<<"Can\'t print stack of length 0";
            }
        }
    private:
        int size;
        int *def_arr(int size)
        {
            int arr[size];
            return arr;
        }
};

And the code that I ran:

int main()
{
    stack A(3);
    A.push(5);
    A.push(8);
    A.push(10);
    cout<<A.arr[1]<<"\n";
    A.print();
}

And the result:

8
5       87      -1259567440

What am I missing?

`def_arr` returns a dangling pointer. Also `int arr[size]` is non-standard C++ — UnholySheep, Aug 10 '18 at 08:53
Variable-length arrays are not part of the C++ standard. Some compilers support them as an extension, but that is not required of them. The standard way is to use `std::vector` — UnholySheep, Aug 10 '18 at 08:55
If you insist on using a pointer you can simply replace that line with `arr = new int[size]();` (you've already defined `arr`), but don't forget to `delete[]` it in the destructor. — Qubit, Aug 10 '18 at 08:56
Possible duplicate of [How to return an array from a function?](https://stackoverflow.com/questions/4264304/how-to-return-an-array-from-a-function) — UnholySheep, Aug 10 '18 at 08:57
But the problem shouldn't be there as I said the elements of the array show up fine individually. It's only when I try to display them using a loop everything messes up. — user8850564, Aug 10 '18 at 08:57
Your issue here is that you define an array inside the function statically. Once the function exits, that array is deleted. Now you're writing to memory that you have not allocated, this leads to undefined behaviour, i.e. anything can happen. Not to mention you use the same name in the same scope, that's also somewhat confusing. — Qubit, Aug 10 '18 at 08:59
@Qubit But it seems to work. As I wrote above individual elements show up fine. — user8850564, Aug 10 '18 at 09:03
That's the beauty of the word **undefined**, it usually works for a while and then it stops when something else tries to use the same memory. — Qubit, Aug 10 '18 at 09:04

john · Accepted Answer · 2018-08-10T09:14:59.393

0

int *def_arr(int size)
    {
        int arr[size];
        return arr;
    }

is wrong because it returns a pointer to a local variable arr. Once the function exits the local variable no longer exists, so the pointer points at an invalid address (aka a dangling pointer). Using that pointer is undefined behaviour.

And a variable length array (VLA) is a non-standard construct in C++.

Two possible solutions

1) Use a std::vector

#include <vector>

std::vector<int> def_arr(int size)
    {
        std::vector<int> arr(size);
        return arr;
    }

This will require changes elsewhere in your code (mainly that arr in your class will have to be redeclared as a std::vector too).

2) Use dynamic memory allocation

int *def_arr(int size)
    {
        int* arr = new int[size];
        return arr;
    }

This will cause memory leaks in your code, fixing these is non-trivial, you should read up on the 'rule of three'.

Most people would recommend option 1. Congratulations you are about to learn about the most important topic in C++, resource management.

edited Aug 10 '18 at 09:14

answered Aug 10 '18 at 09:07

john

85,011
4
57
81

`... is undefined behaviour because it returns a pointer to a local variable arr` *Technically* returning a pointer to a local variable isn't undefined itself. *Indirecting* that pointer has undefined behaviour. – eerorika Aug 10 '18 at 09:08
1

(3) Replace all of this with `std::stack` and avoid reinventing wheels. – WhozCraig Aug 10 '18 at 09:11
Thanks, that worked. But I am still wondering why the program worked when printing a single element but not when the same statement was executed using a loop. Is there something to know about loops that I am missing? – user8850564 Aug 10 '18 at 09:14
As stated, something else tried to use the same memory, i.e. your for loop defines an integer, that might've overwritten one of the values, then you called printf, that might also try to use some of the memory that you've been writing to. So, something saved it's data there and that's what you read, your data was overwritten. – Qubit Aug 10 '18 at 09:17
2

@user8850564 Undefined behaviour is undefined behviour. Your program had undefined behaviour with or without the loop. Undefined behaviour includes the possibility that your program works. This actually makes C++ programming hard because a working program can have undefined behaviour and you don't know, until it suddenly stops working. That's exactly what happened to you. – john Aug 10 '18 at 09:17

Initialised arrays showing different values in a loop

1 Answers1