2

I'm writing a program that gets an array of integers and its logical size. When called it creates a new array containing only the positive numbers from the arrays.

Now, in order to do this I need to write a void type function takes the following parameters:

(int* arr, int arrSize, int** outPosArrPtr, int* outPosArrSizePTR)

I'm supposed to use the pointer int** outPosArrPtr to update the base address of the array containing the positive numbers, and the pointer outPosArrSizePtr to update the array's logical size.

now when I run my code on the xcode compiler the logical size gets updated to a very larger number. So, when I tried to run the program using the online gdb compiler I got the error "Segmentation fault."

From reading what Segmentation fault means I learnt that it means that I'm trying to access memory that "does not belong to me" or memory that is not in the call stack or in the heap portion of the program.

I tried to debug my code by seeing if I was referening any null pointers or see if I was referencing any dangling pointers but it seems like the problem is another one.

My code:

#include <iostream>

typedef int* IntArrPtr;
using namespace std;


int main() {
    int arrSize;
    int *ptrSize;
    ptrSize = &arrSize;

    cout << "How many integers will this array hold:\n ";
    cin >> arrSize;

    IntArrPtr a;
    a = new int[arrSize];

    fillArr(a, arrSize);

    getPosNums4(a, arrSize,&a, ptrSize);
    cout << "The new size in main is: " << arrSize << endl;

    cout <<"The new array with positive integers is:\n";
    /*for(int i =0; i<arrSize;i++) // this runs for a large size of arrSize
        cout<< a[i] << " ";
    cout<<endl; */
    return 0;
}

void fillArr(int a[], int size){
    cout << "Please enter " << size << " Integers separated by spaces\n";
    cout << "Press enter when finished >\n";
    int i;

    for (i=0;i<size;i++)
        cin >> a[i];

}

void getPosNums4(int* arr, int arrSize, int** outPosArrPtr,int* outPosArrSizePtr){
    IntArrPtr newArr;
    newArr = new int[arrSize];
    int i;
    int newIndx = 0;
    outPosArrSizePtr = &newIndx;//initiliaze the pointer.
    for(i=0;i<arrSize;i++){
        if(arr[i] > 0){
            newArr[newIndx] =arr[i];
            newIndx++;
        }
    }
    arrSize = newIndx;
    *outPosArrSizePtr = arrSize;
    cout << "The new size is of *outPosArrSizeptr is: " << *outPosArrSizePtr << endl;

    for(int j=0;j<newIndx;j++)
        outPosArrPtr[j] = &newArr[j];
    delete []newArr;
    newArr = NULL;
    for(int i=0;i<newIndx;i++)
        arr[i] = *outPosArrPtr[i];

}

an example When I run this program on Xcode:

How many integers will this array hold:
 6
Please enter 6 Integers separated by spaces
Press enter when finished >
3 -1 -3 0 6 4
The new size is of *outPosArrSizeptr is: 3
The new array with positive integers is:
The new size in main is: 7445512
The program ended with exit code: 0
Simon
  • 241
  • 2
  • 13
  • 2
    To state the obvious... you should most likely use a `std::vector` in C++. You can use `reserve` to give a size hint for the vector. You can use `push_back` to add elements to the vector after reading them using `cin >> x`. It should not be much more difficult than [How to cin to a vector](https://stackoverflow.com/q/8377660/608639). – jww Sep 17 '18 at 04:21

2 Answers2

1

There are quite a few problems there, but the most crucial one is that assigning a value to a function's argument has no effect on the variable whose value you passed as the argument.
It doesn't matter that the argument is a pointer – there is nothing special about pointers.

What I think is happening is that your "copy back and forth" loop (I can't understand what it's supposed to do) in the function is writing outside the input array, causing undefined behaviour and, in this case, overwriting variables in main.

You're overcomplicating your function quite a bit. It should

  • Create a new array
  • Copy the positive values to this array
  • Update the output parameters with the address of this array and its (logical) size

(Think of out parameters as return values and handle them last.)

Something like this:

void getPosNums4(int* arr, int arrSize, int** outPosArrPtr,int* outPosArrSizePtr){
    int* newArr = new int[arrSize];
    int newIndx = 0;
    for (int i = 0; i < arrSize; i++){
        if (arr[i] > 0){
            newArr[newIndx] = arr[i];
            newIndx++;
        }
    }
    *outPosArrPtr = newArr;
    *outPosArrSizePtr = newIndx;
}

You should also not pass pointers to your "originals" for this function to modify, you should use new variables.

int main() {
    int arrSize = 0;
    cout << "How many integers will this array hold:\n ";
    cin >> arrSize;
    int* a = new int[arrSize];
    fillArr(a, arrSize);
    int * positives = nullptr;
    int positiveSize = 0;
    getPosNums4(a, arrSize, &positives, &positiveSize);
    cout << "The new size in main is: " << positiveSize << endl;

    delete [] a;
    delete [] positives;
}
molbdnilo
  • 64,751
  • 3
  • 43
  • 82
  • `int * positives = nullptr;` is the same as initializing a pointer using the new operator? – Simon Sep 18 '18 at 00:34
  • 1
    @SimonGarfe No, it's initialising it to the null pointer. The memory will be allocated by `getPosNums4`. It's a very good habit to always initialise your variables. – molbdnilo Sep 18 '18 at 04:12
0

Modern C++ uses vector rather than manually allocating arrays. Manual allocation is prone to a variety of errors that are very difficult to debug.

The logic in your getPosNums4 method appears to be the trouble. If I understand the requirement, it should look for positive integers in the input array and copy them to a newly allocated output array. Over-allocating the output array is non-optimal but not an actual bug.

void getPosNums4(int* arr, int arrSize, int** outPosArrPtr,int* outPosArrSizePtr){
    IntArrPtr newArr;
    newArr = new int[arrSize];
    int i;
    int newIndx = 0;
    for(i=0;i<arrSize;i++){
        if(arr[i] > 0){
            newArr[newIndx] =arr[i];
            newIndx++;
        }
    }
    *outPosArrSizePtr = newIndx;
    cout << "The new size is of *outPosArrSizeptr is: " << *outPosArrSizePtr << endl;
    *outPosArrPtr = newArr;
}

Note the the newly allocated array will need to be delete[] by the calling function or a memory leak will result.

Here is the same program in modern C++. Note that there is no use of new/delete which saves a lot of misery.

#include <iostream>
#include <vector>
#include <algorithm>

using namespace std;

int main() {
    vector<int> integer_vector;
    vector<int> positive_vector;

    cout << "Type in integers. Type a Q to continue:" << endl;
    int an_int;
    while(cin >> an_int)
      integer_vector.push_back(an_int);

    for_each(integer_vector.begin(),integer_vector.end(),[&](int const& n){ 
       if(n > 0)
          positive_vector.push_back(n);
    });
    cout <<"The new array with positive integers is:\n";
    for(auto const & element:positive_vector)
        cout<< element << " ";
    cout<<endl; 
    return 0;
}
Matthew Fisher
  • 2,258
  • 2
  • 14
  • 23
  • Is the `for_each` better than the more concise range-for? Or, wouldn’t `copy_if` clearly show intent? – Kuba hasn't forgotten Monica Sep 17 '18 at 04:43
  • The range for is fine but I was reaching for the suite and lamdas, as an introduction for the OP. copy_if has the irritating problem of the vector.reserve() needing to be called beforehand, right? Good thinking though, I'm always learning more here! – Matthew Fisher Sep 17 '18 at 04:52
  • `vector.reserve` is an optimization, or perhaps lack of premature pessimization — depending on point of view. Not calling it cannot change the behavior, other than the number of copies/moves performed, and the number of allocations. You use range-for anyway, it seems. – Kuba hasn't forgotten Monica Sep 17 '18 at 04:57