1

I'm writing a program that returns an array with the indexes of another array as elements when that array's element =1.

So if array[ix]=1 then newarray[newarrcounter]=ix.

However the array that is returned only has 0s as elements.

I'm trying to do this using only pointers and no indexing. Am I using pointers incorrectly here?

int* primearr(int arr[], int size, int& newarrsize){
    int* end = arr + size;
    int* begin = arr;
      while(begin<end){
        if(*begin==1)
          ++newarrsize;
        begin++;
      }
      begin=arr;

    int *newarray= new int[newarrsize];
      while(begin<end){
        if(*begin==1){
          *newarray=begin-arr;
          newarray++;
        }
        begin++;
      }
    return newarray;
}

the rest of the program code...

#include <iostream>
#include <math.h>

using namespace std;
int* arr(int size);
int firstprime(int size, int arr[]);
void crossout(int size, int*arr, int factor);
void findzeros(int arr[], int size);
int* primearr(int arr[], int size,int& newarrsize);


int main()
{
    int low, high;
    char again='y';
    high=low=0;
    int firstn;
    int newarrsize=0;

    cout<<"\tThe Sieve of Eratosthenes"<<endl<<endl;
    do{
    do{
     do{
        cout<<"Enter the high boundary: ";
        cin>>high;
        cout<<endl;
        if(high<=0)
        cout<<"ERROR: HIGH BOUNDARY MUST BE POSITIVE"<<endl;
      }while(high<0);

      do{
        cout<<"Enter the low boundary: ";
        cin>>low;
        cout<<endl;
        if(low<=0)
        cout<<"ERROR: LOW BOUNDARY MUST BE POSITIVE"<<endl;
        }while(low<=0);
     if(low>high)
                cout<<"ERROR: HIGH BOUNDARY MUST BE GREATER THAN LOW BOUNDARY"<<endl;
    }while(low>=high);

    int* thearr= arr(high);

    firstn=firstprime(high,thearr);

    do{
      crossout(high,thearr,firstn);
      firstn=firstprime(high,thearr);
    }while(firstn<= sqrt(high));

    findzeros(thearr,high);

    int* thenewarr= primearr(thearr,high,newarrsize);

    cout<<"The prime numbers from "<<low<<" to "<<high<<" are: "<<endl;
    int* begin = thenewarr;
    int* end = thenewarr+newarrsize;
    while(begin<=end){
      cout<<*thenewarr<<"  ";
      ++begin;
    }

    cout<<endl<<endl;
    cout<<endl<<endl;
    cout<<"Try again with new boundaries? (y/n):"<<endl;
    cin>>again;

    delete[] thearr;
    delete[] thenewarr; 

    }while(again=='y');

   return 0;
}

int* arr(int size){
    int* thearray = new int[size]();
    thearray[0] = -1;
    thearray[1] = -1;
    return thearray;
}

int firstprime(int size, int arr[]){
  int* end = arr + size;
  int* begin = arr;
    while(begin<end){
        if(*begin==0)
            return (begin-arr);
        begin++;
    }
    return -1;
}

void crossout(int size, int*arr, int factor){
    int* end = arr + size;
    int* begin = arr;
    int newfactor=factor+factor;
    while(begin<end){
        if((begin-arr)==factor)
          *begin=1;
        else if((begin-arr)==newfactor){
          *begin=-1;
          newfactor=newfactor+factor;
        }
        begin++;
    }
}

void findzeros(int arr[], int size){
    int* end = arr + size;
    int* begin = arr;
    while(begin<=end){
        if(*begin==0)
            *begin=1;
        begin++;
    }
}

int* primearr(int arr[], int size, int& newarrsize){
    int* end = arr + size;
    int* begin = arr;
      while(begin<end){
        if(*begin==1)
          ++newarrsize;
        begin++;
      }
      begin=arr;

    int *newarray= new int[newarrsize];
      while(begin<end){
        if(*begin==1){
          *newarray=begin-arr;
          newarray++;
        }
        begin++;
      }
    return newarray;
}
17
  • 3
    Do yourself a favor and use std::vector. This type of C++ coding has gone out with the dark ages. Commented Apr 30, 2019 at 23:24
  • 3
    Well no wonder students dump C++ and go right to Java or JavaScript after finishing C++ if C++ is being taught this way. Your professor is probably a C programmer teaching C++, and simply doing what they were taught in C. Commented Apr 30, 2019 at 23:31
  • 2
    I've been teaching on and off for far longer than that, and I insist you don't. Commented Apr 30, 2019 at 23:31
  • 3
    Even though its ancient, I would still appreciate an explanation to what I'm doing wrong so I can atleast understand it. Commented Apr 30, 2019 at 23:38
  • 2
    Here's another vital point that never seems to be taught in school: start with something small and simple that works, then build up. In this case you could try writing a function that creates and returns a small array of a fixed size, say [1,1,1]. A bug like the one in your code would have prevented this, and been much easier to find in such a simple function. Commented Apr 30, 2019 at 23:49

1 Answer 1

1

You are not initializing newarrsize to 0 before calculating the new array's size.

After allocating the new array, when you are then populating it, you are incrementing the pointer to the new array. So, when the function exits, you are not returning a pointer to the front of the new array, you are returning a pointer that is past the end of the new array. Not only does this prevent the caller from accessing the indexes you want, but it also prevents the caller from being able to delete[] the array correctly when done using it.

Try this instead:

int* primearr(int arr[], int size, int& newarrsize) {
    int* begin = arr;
    int* end = begin + size;

    newarrsize = 0;
    while (begin < end) {
        if (*begin == 1)
            ++newarrsize;
        ++begin;
    }

    int *newarray = new int[newarrsize];

    int *ptr = newarray;
    begin = arr;
    while (begin < end) {
        if (*begin == 1) {
            *ptr = begin - arr;
            ++ptr;
        }
        ++begin;
    }

    return newarray;
}

That being said, you really should be using std::vector instead:

std::vector<int> primearr(int arr[], int size) {
    int* begin = arr;
    int* end = begin + size;

    std::vector<int> ret;

    while (begin < end) {
        if (*begin == 1)
            ret.push_back(begin - arr);
        ++begin;
    }

    return ret;
}

If not, at least use std::unique_ptr so you can maintain ownership of the array at all times, and the caller doesn't have to worry about deallocating the array manually:

std::unique_ptr<int[]> primearr(int arr[], int size, int& newarrsize) {
    int* begin = arr;
    int* end = begin + size;

    newarrsize = 0;
    while (begin < end) {
        if (*begin == 1)
            ++newarrsize;
        ++begin;
    }

    std::unique_ptr<int[]> newarray(new int[newarrsize]);
    // or, in C++14 and later:
    // auto newarray = std::make_unique<int[]>(newarrsize);

    int *ptr = newarray.get();
    begin = arr;
    while (begin < end) {
        if (*begin == 1) {
            *ptr = begin - arr;
            ++ptr;
        }
        ++begin;
    }

    return std::move(newarray);
}
Sign up to request clarification or add additional context in comments.

2 Comments

I had to change the last loop to while(begin<=end) so the last element of the new array doesn't = 0, but otherwise thank you so much.
begin < end is correct. end does not point at a valid element of arr but at 1 past the last element (intentional and correct). Dereferencing begin when begin == end would be undefined behavior and likely to crash. Besides, it shouldn't matter if the last element of newarray is 0 or not (unless you have logic errors outside of this function). The only way the last element of newarray could be 0 is if the input size is 1 and the 1st element of arr is 1. The caller is expected to use newarraysize to know how many elements are in the newarray regardless of their values.

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.