EXC_BAD_ACCESS error in C++ array reverser

Question

Below is a simple program to create an array that is the reverse of the passed array. The code throws the following error at line 6: Thread 1: EXC_BAD_ACCESS (code=1, address=0x7fff0000002c). The program compiles and the logic seems fine. Could someone explain my mistake?

void arrayReverser(int array[], int arrayLength) {
    int arrayNew[arrayLength];
    int current;  
    current = array[0];

    for(int i = 0; current != '\0'; current = array[++i]) {
        arrayNew[i] = array[arrayLength - i]; // Thread 1 error code occurs here
    }

    array = arrayNew;
}

int main() {
     int array[] = { 2, 3, 4, 5, 6, 7 };
     arrayReverser(array, 6);
}

Answer 1

It looks like you have two problems. First, your array reverser logic is for looping based on a terminator that does not exist in your array. You should instead loop based on array length.

Second, your arrayReverser function is allocating the return value (arrayNew) on the stack. Stack allocated local variables are automatically released when the method ends. You will need to allocate the return value on the heap to return it with new or malloc - and release it with delete/free in main.

If you prefer to keep the array in main intact, instead of using dynamic allocation, you could assign in place the values instead, like this:

#include <iostream>

void arrayReverser(int array[], int arrayLength) {
    int arrayNew[arrayLength];

    // Reverse the array using array length as terminator
    for( int i=0; i <= arrayLength; i++ ) {
        arrayNew[arrayLength-i] = array[i-1];
    }

    // Assign elements back to original array, instead of assigning array pointer
    for( int i=0; i < arrayLength; i++ ) {
       array[i] = arrayNew[i];
    }
}

int main() {
     int array[] = { 2, 3, 4, 5, 6, 7 };
     arrayReverser(array, 6);

    // Output reveresed array
    for( int i=0; i < 6; i++ ) {
       std::cout << array[i] << std::endl;
    }
}

This works, since it's re-using the sent in array for the result - and your "arrayNew" variable can safely be destroyed without being used. This "deep copy" of the array elements keeps the original array's allocation.

Answer 2

Your issue is the check in the for loop:

current != '\0'

This should be a check against the array length, not the value of an element in an array, this would work to reverse a null terminated string though.

Answer 3

Look at the loop continuation condition in this line:

for(int i = 0; current != '\0'; current = array[++i]) {

You're looping until you meet the NUL ('\0') character (which will have an integer value of 0).

The array you passed in doesn't have a zero 'sentinel' at the end. It looks like you meant to use the arraylength parameter and then had a 'brain fart'.

The code just runs off the end of the array and then executes illegal access somewhere.

PS: Please use size_t for array length parameters. It's what it's there for.