-1

I have to read from a txt file these lines:
1 334.5909245845 161.7809319139
2 397.6446634067 262.8165330708
3 503.8741827107 172.8741151168
4 444.0479403502 384.6491809647
5 311.6137146746 2.0091699828
6 662.8551011379 549.2301263653
7 40.0979030612 187.2375430791

From here I have to extract the second and third values of each line because they're the coordinates for my cities. My piece of code is the following one (I will show only the part where the program has to read the values):

#include <stdio.h>
#include <string.h>
#include <math.h>
#include <time.h>
#include <stdlib.h>
#include <ctype.h>
#include <stdlib.h>


struct city {
    double x;
    double y;    
};

int main(int argc, const char *argv[])
{

    FILE *f;
    f = fopen(argv[1], "r");
    if (f == NULL)
    {
        printf("cannot read file or inexistant file.\n");
        return 1;
    }
    char lines[2000][150];
    int i = 0;
    int j = 0;
    int c = 0;
    // read file's lines
    while (c != EOF)
    {
        while ((c = getc(f)) != EOF && c != '\n')
        {
            lines[i][j] = c;
            j++;
        }
        lines[i][j] = '\n';
        i++;
        j = 0;
    }
    strcpy(lines[i - 2], "\n");


    struct city * cities = malloc(sizeof(double) * 10 + 1);
    if (cities == NULL)
    {
        printf("cannot allocate memory");
        return 1;
    }

    int counter = 0;
    for (int y = 0; strcmp(lines[y], "\n") != 0; y++)
    {
        // don't need this check
        if (isdigit(lines[y][0]))
        {
            char * tok;
            struct city *new_city = malloc(sizeof(struct city));
            if (new_city == NULL)
            {
                printf("cannot allocate memory");
                free(cities);
                return 1;
            }
            //read first number, not used
            tok = strtok(lines[y], " ");

            //read coordinate x
            tok = strtok(NULL, " ");
            printf("tok1: %s\n", tok);
            new_city -> x = atof(tok);

            //read coordinate y
            tok = strtok(NULL, " ");

            printf("tok2: %s\n", tok);
            new_city -> y = atof(tok);
            printf("inserted: %lf\n", new_city -> y);
            cities[counter] = *new_city;
            counter++;
        }
    }
   fclose(f);

Simply I open the file, read char by char all lines, and then I use strtok() to take the coordinates written (second and third numbers of each line). The problem is that I have to store them into x and y of my city struct, and as I read here atof() must be used, but it approximate the number and then it return segmentation fault, as I printed here (inserted is city->y that is approximated but it is wrong, while tok1 and tok2 are the two correct strings read form the file):

tok1: 334.5909245845
tok2: 161.7809319139

inserted: 161.780932
tok1: 397.6446634067
tok2: 262.8165330708

inserted: 262.816533
tok1: 503.8741827107
tok2: 172.8741151168

inserted: 172.874115
tok1: 444.0479403502
tok2: 384.6491809647

zsh: segmentation fault  ./Travelling_salesman_problem ch130.tsp

As you can see comparing the inserted value and tok2, inserted is approximated and then the code breaks. There is a way without changing the code but only the atof() function to have the precise value (because the rest of the code works)?

Ele975
  • 352
  • 3
  • 13
  • Does this answer your question? [converting string to a double variable in C](https://stackoverflow.com/questions/10075294/converting-string-to-a-double-variable-in-c) – eglease Dec 17 '21 at 14:08
  • Seems like you’re providing null pointer to the printf. Make sure you allocate enough memory, and check for null pointer. – Lior Pollak Dec 17 '21 at 14:13
  • 1
    `struct city * cities = malloc(sizeof(double) * 10 + 1);` This allocates memory for 10 elements of type `double` (plus 1 useless extra byte). But it seems to me you want N elements of type `struct city`. – Steve Summit Dec 17 '21 at 14:15
  • While not simply reading one integer then two doubl, instead of strings? – Damien Dec 17 '21 at 14:16
  • @eglease I tried to use sscanf(lines[y], "%*d %lf %lf", &new_city->x, &new_city->y) and also strtok() because I saw the post you said, but they return the same result. Also if I print them with %f it returns 0, anyway I have to insert them in a double variable. – Ele975 Dec 17 '21 at 14:16
  • `lines[i][j] = '\n';` --> `lines[i][j] = '\0';` I think. At least, I don't see you `NUL` terminating your strings anywhere, so who knows what the `str*` functions are doing. Look at what you read from file with a debugger. – yano Dec 17 '21 at 14:18
  • @Steve Summit II tried to change it inserting the struct but the result is still the same. I need that additional byte for later. – Ele975 Dec 17 '21 at 14:20
  • @yano It doesn't change anything, I think putting '\0' or '\n' is not a problem for the compiler – Ele975 Dec 17 '21 at 14:22
  • Printing with `%lf` will only ever print 6 decimal places. If you want more, you tell it what you want: `%.10lf` will print 10 decimal places. – Jonathan Leffler Dec 17 '21 at 14:23
  • 3
    @ElenaFranchini Please, if you come here asking for help, and we offer suggestions, *listen* to our suggestions! We do know what we're talking about, and these suggestions we're giving you are valid. That thing you're doing with `\n`, that yano was asking about, cannot be right. The 10 doubles you're allocating are definitely not enough: your sample data will require 14. If narrow fixes to those issues haven't fixed everything, it's because your code has multiple problems. But those things definitely need fixing also, before the code can ever work. – Steve Summit Dec 17 '21 at 14:25
  • @Jonathan Leffler you're right, using %.10lf I can see the other digits, but now the problem is the segmentation fault, because the rest of the code is supposed to work with double values, then I don't understand why it doesn't continue to store the coordinates taken in city.x and city.y – Ele975 Dec 17 '21 at 14:28
  • @Steve Summit I'm sorry, I don't want to seem ungrateful, but I already tried everything you tell me. I already changed the terminal of the string into '\0', and I said that maybe there is no problem to use '\n' because the result didn't change. I also changed the allocated memory in: cities = malloc(sizeof(struct city) * 100 + 1), only to be sure, but it didn't fix my problem. – Ele975 Dec 17 '21 at 14:30
  • @ElenaFranchini All I can say is, there are multiple things wrong with this code. I tried it on my computer, and it worked fine, no segmentation fault at all — but that doesn't mean the code is fine, because we know it gets a segmentation fault on your computer. So you must have some undefined behavior somewhere. – Steve Summit Dec 17 '21 at 14:37
  • 1
    If you plan to allocate 10 city structures, this is the wrong way to do it: `struct city * cities = malloc(sizeof(double) * 10 + 1);` — You should be using `sizeof(cities[0])` instead of `sizeof(double)` and the `+ 1` is voodoo programming. Your crash comes from trying to store 7 cities (14 doubles) into an array that can only hold 5 cities (10 doubles). Since you know about `malloc()`, you also know about `realloc()`, and you can arrange to allocate the array incrementally, doubling the number of rows each time. You need two counters — number of cities allocated and number of cities used. – Jonathan Leffler Dec 17 '21 at 14:38
  • 2
    There's a big difference between syntax errors and runtime errors, as you've seen with your segfault. You're absolutely correct the compiler doesn't care whether you "terminate" your strings with `'\n'`, `'\0'`, or any other character. But `strcpy` and `strcmp` _do_ care. These functions work with _strings_, which in C are defined as a sequence of 1-or-more `char`s ending with `'\0'`. Feeding improperly formatted strings to the `str*` functions invokes [undefined behavior](https://en.wikipedia.org/wiki/Undefined_behavior), which can manifest itself as a segfault. – yano Dec 17 '21 at 14:39
  • 1
    Besides the problems that have been mentioned, the code is quite a bit more complicated than it needs to be. You don't need to read the whole file into a two-dimensional `lines` array — you could read a line at a time. You could read lines of text by calling `fgets` instead of doing it a character at time. You don't need to allocate `new_city` each time. I'm not saying that any of these things are the direct cause of your problem, but by being more complicated than they have to be they raise the odds of having problems, and they make it harder to spot where the real problem is. – Steve Summit Dec 17 '21 at 14:40

1 Answers1

2

So many problems.

It is unclear why all the lines are read into char lines[][]; first and then parsed. It makes more sense to read 1 line, parse it into the data structure and then read the next line, etc. Then only one line buffer needed.

Yet assuming there is need to read the entire file first, some code to fix input:

// Avoid naked magic numbers
//  char lines[2000][150];
#define LINES_N 2000
#define LINE_SIZE 150
char lines[LINES_N][LINE_SIZE];

int i = 0;  // Maybe line_count instead?
int j = 0;  // Maybe ch_index instead?
int c = 0;  // Good use of int here.


// Append a '\0' at the end to form a _string_.
// Use 1 while loop
// Do not iterate to far, add limits tests

// read file's lines
while (i < LINES_N && (c = getc(f)) != EOF) {
  if (j < LINE_SIZE) {
    lines[i][j] = c;
    j++;
  } else {
    fprintf(stderr, "Line %d too long\n", i);
  }    
  if (c == '\n') { 
    lines[i][j] = '\0';
    i++;
    j = 0;
  }
}

// Might as well close the file now, not later.
fclose(f);  // and remove later one.

// lines[i - 2] is very bad if i < 2
//strcpy(lines[i - 2], "\n");
// I think all you want here is 
// strcpy(lines[i], "\n");
// Since you have `i`, later iterate to `i`,
//   no need for strcpy(lines[i], "\n")
//
// for (int y = 0; strcmp(lines[y], "\n") != 0; y++)
for (int y = 0; y < i; y++)

If helpful, later I'll look over the rest.

chux - Reinstate Monica
  • 143,097
  • 13
  • 135
  • 256