0

I'm not sure why I'm receiving this error. I was wondering could it be due to my terminal not being able to read the txt files that run with this program. Let me know if that could be a possible reason for that message.

Im mainly just looking for syntax errors. The files I have just contain a big bunch of numbers and I'm supposed to work from the third value onwards {the first two have another use)

#include <stdlib.h> 

#define N 1000000

int main(void)
{

int n;  /* The number of lengths */
int x; /* The target length */
int lengths[N]; /* The array of available lengths */
int i, j;
int whichfile=1;

FILE *fp;

scanf("%d", &whichfile);

switch (whichfile) {
   case 1:
      fp = fopen("testcase_small_sorted.txt", "r");
      break;
  case 2:
      fp = fopen("testcase_large_sorted.txt", "r");
      break;
  case 3:
      fp = fopen("testcase_small_nomatch_sorted.txt","r");
      break;
  case 4:
      fp = fopen("hidden_small_sorted.txt","r");
      break;
  case 5:
      fp = fopen("hidden_large_sorted.txt","r");
      break;

}

fscanf(fp, "%d", &x);
fscanf(fp, "%d", &n);

for (i=0;i<n;i++)
  fscanf(fp, "%d", &lengths[i]);


fclose(fp);

/* Now all the input data has been read in
   search for the required pair of lengths... */

x = lengths[0];
n = lengths[1];

for(i = 2; i < n; i++)
{
for(j = 2; i < n; j++)
{
  if(lengths[i] + lengths[j] == x)
  {
    printf("Found: %d + %d == %d\n", lengths[i], lengths[j], x);
  }
}
}

return 0;

} ```

  • Yes, if the file cannot be opened, `fopen` will return NULL and the following `fscanf` will attempt to dereference the NULL pointer, which typically causes a segmentation fault. You should check the value returned from `fopen` and do appropriate error handling if it is NULL. – Nate Eldredge Nov 29 '20 at 23:46
  • how would I go by doing that, if you don't mind explaining. – Chris Thomas Nov 29 '20 at 23:46
  • You should also include a `default:` case for if the input is something other than 1 through 5, as your program will also crash in that case. Basically you need proper return value checks and error handling throughout the program. – Nate Eldredge Nov 29 '20 at 23:47
  • `if (fp == NULL) { fprintf(stderr, "Could not open file %s: %s\n", filename, strerror(errno); exit(2); }` – Nate Eldredge Nov 29 '20 at 23:48
  • the 1-5 was input from my teacher so I cant change any of that. – Chris Thomas Nov 29 '20 at 23:48
  • 1
    Also, you have a 4 megabyte array on the stack, which may crash all by itself on some platforms. https://stackoverflow.com/questions/571945/getting-a-stack-overflow-exception-when-declaring-a-large-array/571961#571961 As a quick fix, consider making it `static`. As a better fix, learn about dynamic allocation and allocate precisely the amount you need to hold the data from the file. – Nate Eldredge Nov 29 '20 at 23:53

1 Answers1

1

I'm mainly just looking for syntax errors.

Syntax errors are only the beginning. C will not check anything for you. You have to check if files opened, if input scans worked, if values are inside array bounds. If you don't, that's how you get segfaults.

For files, the usual pattern is to try to open the file, check its return value, and then handle the error. fopen returns NULL on error and sets errno. errno is a global which holds what sort of error happened like "file not found", but it's a number. strerror is handy to turn it into an error message. Finally fprintf is like printf but can print to things other than stdout. In this case stderr. Both stdout and stderr normally appear on the screen, but they can be separated.

FILE *fp = fopen(path, mode);

if( fp == NULL ) {
    // This will print something like "Could not open testcase_small_sorted.txt: No such file or directory"
    fprintf(stderr, "Could not open file %s: %s", path, strerror(errno));
    exit(1);
}

But now we need to copy this five times because the code repeats fopen. Instead of each case opening the file, what if it just picked a filename? And since each filename has a number, what if they were just in an array?

    const char *files[] = {
        NULL, // 0 is not used
        "testcase_small_sorted.txt",
        "testcase_large_sorted.txt",
        "testcase_small_nomatch_sorted.txt",
        "hidden_small_sorted.txt",
        "hidden_large_sorted.txt"
    };

    const char *path = files[whichfile];

What if they input something that's not a number? Or what if the number is out of range? These need to be checked for as well. scanf will return the number of items matched. We expect 1. If we get anything else it didn't work.

    // Check that we read an integer.
    if( scanf("%d", &whichfile) != 1 ) {
        fprintf(stderr, "Please enter 1-5.\n");
        exit(1);
    }
    // Check that it's in range.
    if( (whichfile < 1) || (5 < whichfile) ) {
        fprintf(stderr, "Please enter 1-5.\n");
        exit(1);
    }

Note that scanf has a lot of problems and should be swiftly discarded once you learn things like fgets and sscanf.


int lengths[1000000] is 4 to 8 megabytes (1 million integers at 4 or likely 8 bytes per integer) and might get you an eponymous stack overflow. Your algorithm is O(n^2) which means if there really were 1,000,000 items it would take 1,000,000,000,000 iterations to find all the pairings and your class will probably be done before the program is.

(A little hint to improve the algorithm: if the numbers are sorted, and you're looking for two of them to sum X, do you need to check every number?)

I think you're meant trust n from the file for how many to read. Normally you don't have this, it's a crutch to let you read input without using dynamic memory (you'll be taught that later). Even if you had it, you wouldn't trust it anyway in production code; if it lies you'll walk out of your array bounds. But for this exercise that's fine.

    // Read the first two lines, the sum and the number of remaining lines.
    // Note that `fscanf` can also fail and needs to be error checked.
    // I'll leave that as an exercise.
    fscanf(fp, "%d", &x);
    fscanf(fp, "%d", &n);

    // Use `n` to allocate just enough space on the stack.
    int lengths[n];

    // Read the rest.
    // Use braces, even for one line.
    // They avoid a very silly and hard to debug mistake.
    for (i=0;i<n;i++) {
        fscanf(fp, "%d", &lengths[i]);
    }

    fclose(fp);

There's no need to put x and n into lengths. They aren't meant to be summed and you skip over them anyway by starting your loops at 2.

With that fixed the loops can start at 0. There is a mistake in the inner loop, it should check j < n not i < n.

    for(i = 0; i < n; i++)
    {
        for(j = 0; j < n; j++)  // <<--- j < n, not i < n.
        {
            if(lengths[i] + lengths[j] == x)
            {
                printf("Found: %d + %d == %d\n", lengths[i], lengths[j], x);
            }
        }
    }
Schwern
  • 153,029
  • 25
  • 195
  • 336