Function returning array with rubbish values

Question

I am trying to allocate an array on the heap so that I can return it as the output of a function that is supposed to reverse the order of the elements. when I run the program, however, The first element of array1 is missing and I get rubbish at the start of array2. Am I declaring the array wrong?

Also, since I am working with dynamic memory, must I release the memory with the delete command or will it be deleted automatically as it is within the local scope of the reverseArray function?

#include <iostream>

unsigned *reverseArray(unsigned *arr)
{
    unsigned *output = (unsigned*) malloc(sizeof(int)*5);

    for(unsigned i = 0; i < 5; ++i)
        output[i] = arr[5 - i];

    return output;
}

int main()
{
    unsigned array1[5] = {10, 20, 30, 40, 50};

    unsigned *array2 = reverseArray(array1);

    for(unsigned i = 0; i < 5; ++i)
        std::cout << array2[i] << " ";

    std::cout << std::endl;
    
    return 0;
}

The output I get is

32751 50 40 30 20 

Answer 1

As pointed out in the comments, your index was wrong: 5-i accesses fields 5,4,3,2,1. Leaving out 0 and much more importantly, accessing past the end of the array.

However, you can just use the standard library's std::reverse function template, which even operates in-place:

#include <algorithm>
#include <iterator>
#include <iostream>

int main()
{
    unsigned array1[5] = {10, 20, 30, 40, 50};

    std::reverse(std::begin(array1), std::end(array1));

    for(auto const& value: array1)
        std::cout << value << ' ';
    std::cout << '\n';
}

In case you want to keep the original array, use std::array, which has a handy copy constructor:

#include <algorithm>
#include <array>
#include <iostream>

int main()
{
    std::array<unsigned,5> const array1{10, 20, 30, 40, 50};
    auto array2 = array1;

    std::reverse(std::begin(array2), std::end(array2));

    for(auto const& value: array1) std::cout << value << ' ';
    std::cout << '\n';
    for(auto const& value: array2) std::cout << value << ' ';
    std::cout << '\n';
}

This has the advantage that no dynamic allocations are performed. std::reverse also operates entirely in-place, so there is nothing to clean up afterwards.

If you have an array of values that is not known at compile time, like your array1 is, you can use std::vector which takes care of mopping up memory after you're done with it. std::reverse works with that as well.

Answer 2

The problem is in this line:

output[i] = arr[5 - i];

Since i will get the values [0,1,2,3,4], 5 - i will get the values [5,4,3,2,1].
In order to get 0 based indices ([4,3,2,1,0]) as required, you need to subtract 1:

output[i] = arr[4 - i]; // 5 - i - 1

A side note:
It's better to use std::vector for dynamic arrays in C++.
Among other advantages, it will save you the need (and potential bugs) of manual new and delete (or malloc and free although they are altogether discouraged in c++ in favor of new/delete in the rare case they are needed).

Answer 3

If you are writing a C++ program then use the operator new instead of the C function malloc.

For example

unsigned *output = new unsigned[5];

And you always should free all the allocated memory when it is not required any more.

Your function has undefined behavior because in this statement

    output[i] = arr[5 - i];

when i is equal to 0 there is an access to memory outside the passed array because in this case the statement is look like

    output[0] = arr[5];

Also the function parameter should have the qualifier const because passed arrays are not changed within the function.

Also using the magic number 5 within the function makes the function useless.

The program can look the following way

#include <iostream>
#include <iterator>

unsigned * reverseArray( const unsigned *arr, size_t n )
{
    unsigned *output = nullptr;

    if ( n != 0 )
    {
        output = new unsigned[n];

        for ( std::size_t i = 0; i < n; ++i )
        {
            output[i] = arr[n - i - 1];
        }
    }

    return output;
}

int main()
{
    unsigned array1[] = { 10, 20, 30, 40, 50 };
    const std::size_t N = std::size( array1 );

    unsigned *array2 = reverseArray( array1, N );

    for ( std::size_t i = 0; i < N; i++ )
    {
        std::cout << array2[i] << ' ';
    }
    std::cout << std::endl;
    
    delete [] array2;

    return 0;
}

Pay attention to that there is standard algorithm std::reverse_copy that can be used. Using this algorithm the program can look the following way

#include <iostream>
#include <iterator>
#include <algorithm>    

int main()
{
    unsigned array1[] = { 10, 20, 30, 40, 50 };
    const std::size_t N = std::size( array1 );
    unsigned array2[N];

    std::reverse_copy( std::begin( array1 ), std::end( array1 ),
                       std::begin( array2 ) );

    for ( const auto &item : array2 )
    {
        std::cout << item << ' ';
    }
    std::cout << std::endl;

    return 0;
}

There is no need to allocate dynamically a new array. Otherwise you could write for example

#include <iostream>
#include <iterator>
#include <algorithm>    

int main()
{
    unsigned array1[] = { 10, 20, 30, 40, 50 };
    const std::size_t N = std::size( array1 );
    unsigned *array2 new unsigned[N];

    std::reverse_copy( std::begin( array1 ), std::end( array1 ),
                       array2 );

    for ( std::size_t i = 0; i < N; i++ )
    {
        std::cout << array2[i] << ' ';
    }
    std::cout << std::endl;

    delete [] array2;

    return 0;
}

If you need to reverse the source array then use another algorithm std::reverse the following way

std::reverse( std::begin( array1 ), std::end( array1 ) );