Getting sementation fault (core dumped)

Question

Everything seems to run okay up until the return part of shuffle_array(), but I'm not sure what.

 int * shuffle_array(int initialArray[], int userSize)
 {
     // Variables
     int shuffledArray[userSize];       // Create new array for shuffled
     srand(time(0));

     for (int i = 0; i < userSize; i++)     // Copy initial array into new array
    {
          shuffledArray[i] = initialArray[i];
    }

     for(int i = userSize - 1; i > 0; i--)
    {
         int randomPosition = (rand() % userSize;
         temp = shuffledArray[i];
         shuffledArray[i] = shuffledArray[randomPosition];
         shuffledArray[randomPosition] = temp;
    }

    cout << "The numbers in the initial array are: ";
    for (int i = 0; i < userSize; i++)
    {
         cout << initialArray[i] << " ";
    }
    cout << endl;
    cout << "The numbers in the shuffled array are: ";
    for (int i = 0; i < userSize; i++)
    {
        cout << shuffledArray[i] << " ";
    }
    cout << endl;
    return shuffledArray;
}

Sorry if spacing is off here, not sure how to copy and past code into here, so I had to do it by hand. EDIT: Should also mention that this is just a fraction of code, not the whole project I'm working on.

Answer 1

There are several issues of varying severity, and here's my best attempt at flagging them:

int shuffledArray[userSize];

This array has a variable length. I don't think that it's as bad as other users point out, but you should know that this isn't allowed by the C++ standard, so you can't expect it to work on every compiler that you try (GCC and Clang will let you do it, but MSVC won't, for instance).

srand(time(0));

This is most likely outside the scope of your assignment (you've probably been told "use rand/srand" as a simplification), but rand is actually a terrible random number generator compared to what else the C++ language offers. It is rather slow, it repeats quickly (calling rand() in sequence will eventually start returning the same sequence that it did before), it is easy to predict based on just a few samples, and it is not uniform (some values have a much higher probability of being returned than others). If you pursue C++, you should look into the <random> header (and, realistically, how to use it, because it's unfortunately not a shining example of simplicity).

Additionally, seeding with time(0) will give you sequences that change only once per second. This means that if you call shuffle_array twice quickly in succession, you're likely to get the same "random" order. (This is one reason that often people will call srand once, in main, instead.)

for(int i = userSize - 1; i > 0; i--)

By iterating to i > 0, you will never enter the loop with i == 0. This means that there's a chance that you'll never swap the zeroth element. (It could still be swapped by another iteration, depending on your luck, but this is clearly a bug.)

int randomPosition = (rand() % userSize);

You should know that this is biased: because the maximum value of rand() is likely not divisible by userSize, you are marginally more likely to get small values than large values. You can probably just read up the explanation and move on for the purposes of your assignment.

return shuffledArray;

This is a hard error: it is never legal to return storage that was allocated for a function. In this case, the memory for shuffledArray is allocated automatically at the beginning at the function, and importantly, it is deallocated automatically at the end: this means that your program will reuse it for other purposes. Reading from it is likely to return values that have been overwritten by some code, and writing to it is likely to overwrite memory that is currently used by other code, which can have catastrophic consequences.

Of course, I'm writing all of this assuming that you use the result of shuffle_array. If you don't use it, you should just not return it (although in this case, it's unlikely to be the reason that your program crashes).

Inside a function, it's fine to pass a pointer to automatic storage to another function, but it's never okay to return that. If you can't use std::vector (which is the best option here, IMO), you have three other options:

• have shuffle_array accept a shuffledArray[] that is the same size as initialArray already, and return nothing;
• have shuffle_array modify initialArray instead (the shuffling algorithm that you are using is in-place, meaning that you'll get correct results even if you don't copy the original input)
• dynamically allocate the memory for shuffledArray using new, which will prevent it from being automatically reclaimed at the end of the function.

Option 3 requires you to use manual memory management, which is generally frowned upon these days. I think that option 1 or 2 are best. Option 1 would look like this:

void shuffle_array(int initialArray[], int shuffledArray[], int userSize) { ... }

where userSize is the size of both initialArray and shuffledArray. In this scenario, the caller needs to own the storage for shuffledArray.

Answer 2

You should NOT return a pointer to local variable. After the function returns, shuffledArray gets deallocated and you're left with a dangling pointer.

Answer 3

When you define any non static variables inside a function, those variables will reside in function's stack. Once you return from function, the function's stack is gone. In your program, you are trying to return a local array which will be gone once control is outside of shuffle_array().

To solve this, either you need to define the array globally (which I won't prefer because using global variables are dangerous) or use dynamic memory allocation for the array which will create space for the array in heap rather than allocating the space on the function's stack. You can use std::vectors also, if you are familiar with vectors.

To allocate memory dynamically, you have to use new as mentioned below.

int *shuffledArray[] = new int[userSize];

and once you completed using shuffledArray, you need to free the memory as below.

delete [] shuffledArray;

otherwise your program will leak memory.

Answer 4

You cannot return a local array. The local array's memory is released when you return (did the compiler warn you about that). If you do not want to use std::vector then create yr result array using new

int *shuffledArray = new int[userSize];

your caller will have to delete[] it (not true with std::vector)