Keep Getting a Segmentation Fault On This?

Question

I keep getting the segmentation fault (core dumped) on the code below. Any ideas on why this is happening. The code is designed to read numbers from a text document, convert them to integers, perform radix sort, and print out the array.

#include <cstdlib>
#include <iostream>
#include <iomanip>
#include <fstream>
#include <string>
#include <time.h>
#include <sstream>

using namespace std;

int getMax(int arr[], int n)
{
    int max = arr[0];
    for (int i = 1; i < n; i++)
        if (arr[i] > max)
            max = arr[i];
    return max;
}

void countSort(int arr[], int n, int exp)
{
    int output[n];
    int i, count[10] = {0};
    for (i = 0; i < n; i++)
        count[(arr[i] / exp) % 10]++;
    for (i = 1; i < 10; i++)
        count[i] += count[i - 1];
    for (i = n - 1; i >= 0; i--)
    {
        output[count[(arr[i] / exp) % 10] - 1] = arr[i];
        count[(arr[i] / exp) % 10]--;
    }
    for (i = 0; i < n; i++)
        arr[i] = output[i];
}

void radixsort(int arr[], int n)
{
    clock_t clockStart;
    clockStart = clock();

    int m = getMax(arr, n);
    for (int exp = 1; m / exp > 0; exp *= 10)
        countSort(arr, n, exp);

    cout << "\nTime taken by radix sort: " << (double)(clock() - clockStart) / CLOCKS_PER_SEC << endl;
}

int StrToInt(string sti) 
{
    int f;
    stringstream ss(sti); //turn the string into a stream
    ss >> f;
    return f;
}

int main()
{
    int arr[10000];
    int i = 0;
    int result;
    string line = "";

    ifstream myfile;
    myfile.open("integers2.txt");
    if(myfile.is_open())
    {
        while(!myfile.eof())
        {
            getline(myfile, line);
            result = StrToInt(line);
            arr[i] = result;
            //cout<< arr[i] <<"\n";
            i++;
        }
    }


    int n = sizeof(arr)/sizeof(arr[0]);
    radixsort(arr, n);

    for (int i = 0; i < n; i++)
    {
        cout << arr[i] << "\n";
    }

    return 0;
}

Contents of the text file I am using for input: 1244 3455 6565 55 765 8768 687 879

Answer 1

Your program has undefined behavior, because it uses more entries of the array than you initialized with data. You pass the length of the entire array for n even though only a small portion of it, from 0 to i, has been initialized.

Change the code to use n in place of i in the reading loop, and pass that n unmodified to the sort function. This is going to fix the problem (demo).

int n = 0;
myfile.open("integers2.txt");
if(myfile.is_open()) {
    while (myfile >> arr[n]) {
        n++;
    }
}
radixsort(arr, n);

Answer 2

Here's your working code:

#include <cstdlib>
#include <iostream>
#include <iomanip>
#include <fstream>
#include <string>
#include <time.h>
#include <sstream>

using namespace std;

int getMax(int arr[], int n)
{
    int max = arr[0];
    for (int i = 1; i < n; i++)
        if (arr[i] > max)
            max = arr[i];
    return max;
}

void countSort(int arr[], int n, int exp)
{
    int output[n];
    int i, count[10] = {0};
    for (i = 0; i < n; i++)
        count[(arr[i] / exp) % 10]++;
    for (i = 1; i < 10; i++)
        count[i] += count[i - 1];
    for (i = n - 1; i >= 0; i--)
    {
        output[count[(arr[i] / exp) % 10] - 1] = arr[i];
        count[(arr[i] / exp) % 10]--;
    }
    for (i = 0; i < n; i++)
        arr[i] = output[i];
}

void radixsort(int arr[], int n)
{
    clock_t clockStart;
    clockStart = clock();

    int m = getMax(arr, n);
    for (int exp = 1; m / exp > 0; exp *= 10)
        countSort(arr, n, exp);

    cout << "\nTime taken by radix sort: " << (double)(clock() - clockStart) / CLOCKS_PER_SEC << endl;
}

int StrToInt(string sti) 
{
    int f;
    stringstream ss(sti); //turn the string into a stream
    ss >> f;
    return f;
}

int main()
{
    const int MAX_SIZE = 10;

    int arr[ MAX_SIZE ] = { 0 };

    //int i = 0;
    //int result = 0;
    string line = "";

    ifstream myfile;
    myfile.open("integers2.txt");
    if(!myfile.is_open())
    {
        cerr << "Could not open file!\n";
        return -1;
    }

    cout << "Reading integers...\n";

    int index = 0;
    //while ( index < SIZE && getline( myfile, line ) )
    while ( index < MAX_SIZE && myfile >> arr[ index ] )
    {
        //getline( myfile, line );
        //result = StrToInt( line );
        //arr[index] = std::stoi( line );
        cout << arr[index] <<"\n";
        index++;
    }

    cout << "Sorting integers...\n";

    //int n = sizeof(arr) / sizeof(arr[0]);

    radixsort( arr, index );

    for ( int i = 0; i < index; i++ )
    {
        cout << arr[i] << "\n";
    }

    return 0;
}

Some points:

1. Check std::stoi for string to integer conversion; BTW, you don't need to do that. Just read it directly like this: while ( file >> integer ).
2. Need to check if the file is open; return ERROR otherwise; in your case, the rest of the code was executing anyway even if the file was not open i.e. code after if ( myfile.open() ) { ... }
3. while( !myfile.eof() ) is bad practice. See: Why is iostream::eof inside a loop condition considered wrong?
4. You don't need to calculate the size like int n = sizeof(arr) / sizeof(arr[0]); because you already know the size. Just use a const for this.
5. While reading from file, you also need to validate the maximum size of your array. You should read what the size allows you. Take care of out-of-bounds read / write errors.
6. Use <ctime> instead of <time.h>.