0

I am trying to read lines from a file and store them in a multidimensional char pointer. When I run my code, it runs without errors, however, the lines will not be printed correctly in my main() function, but prints correctly in the getline() function. Can anybody explain what is happening and how I correctly store them in my multidimensional pointer?

My code:

int main (int argc, const char * argv[]) {
    char **s = (char**) malloc(sizeof(char*) * 1000);
    int i = 0;
    while (getline(s[i]) != -1){
        printf("In array: %s \n", s[i]);
        i++;
    }
    return 0;
}

int getline(char *s){
    s = malloc(sizeof(char*) * 1000);
    int c, i = 0;
    while ((c = getchar()) != '\n' && c != EOF) {
        s[i++] = c;
    }
    s[i] = '\0';
    printf("String: %s \n", s);
    if (c == EOF) {
        return -1;
    }
    return 0;
}

and my output:

String: First line
<br>In array: (null) 
<br>String: Second line 
<br>In array: (null) 
<br>String: Third line 
<br>In array: (null)
Improve this question
1
  • Why do you handle the cast of the result of malloc differently in your code? However: do not cast its result. Commented Jan 20, 2014 at 22:59

4 Answers 4

Reset to default
2

You are passing by value. Even though you change the value of *s in getline(), main does not see it. You have to pass the address of s[i] so that getline() can change it:

int getline(char **s) {
  * s= malloc( sizeof(char) * 1000) ;
  ...
}

Also, if you want to be a bit more efficient with memory, read the line into a local buffer (of size 1000) if you want. Then when you are done reading the line, allocate only the memory you need to store the actual string.

int getline( char ** s )
{
  char tmpstr[1000] ;
  ...
    tmpstr[i++]= c ;
  }
  tmpstr[i]= '\0' ;

  * s= strdup( tmpstr) ;
  return 0 ;
}

If you want to improve things even further, take a step back and thing about a few things. 1) allocating the two parts of the multi-dimensional array in two different functions is going to make it harder for others to understand. 2) passing in a temporary string from outside to getline() would allow it to be significantly simpler:

int main()
{
  char ** s= (char **) malloc( 1000 * sizeof(char *)) ;
  char tmpstr[1000] ;
  int i ;

  while ( -1 != getline( tmpstr))
  {
    s[i ++]= strdup( tmpstr) ;
  }

  return 0 ;
}

int getline( char * s)
{
  int c, i = 0 ;
  while (( '\n' != ( c= getchar())) && ( EOF != c )) { s[i ++]= c ; }
  s[i]= '\0' ;

  return ( EOF == c ) ? -1 : 0 ;
}

Now, getline is just about IO, and all the allocation of s is handled in one place, and thus easier to reason about.

Improve this answer
Sign up to request clarification or add additional context in comments.

2 Comments

And how would the main() function look like?
In case 2, main would call getline() with &(s[i]) instead of s[i].
1

The problem is that this line inside getline function

s = malloc(sizeof(char) * 1000); // Should be sizeof(char), not sizeof(char*)

has no effect on the s[i] pointer passed in. This is because pointers are passed by value.

You have two choices here:

  • Move your memory allocation into main, and keep passing the pointer, or
  • Keep your allocation in getline, but pass it a pointer to pointer from main.

Here is how you change main for the first option:

int main (int argc, const char * argv[]) {
    char **s = malloc(sizeof(char*) * 1000);
    int i = 0;
    for ( ; ; ) {
        s[i] = malloc(sizeof(char) * 1000);
        if (getline(s[i]) == -1) break;
        printf("In array: %s \n", s[i]);
        i++;
    }
    return 0;
}
Improve this answer

Comments

0

My advise: completely avoid writing your own getline() function, and avoid all fixed size buffers. In the POSIX.1-2008 standart, there is already a getline() function. So you can do this:

//Tell stdio.h that we want POSIX.1-2008 functions
#define _POSIX_C_SOURCE 200809L
#include <stdio.h>

int main() {
    char** s = NULL;
    int count = 0;
    do {
        count++;
        s = realloc(s, count*sizeof(char*));
        s[count-1] = NULL;    //so that getline() will allocate the memory itself
    } while(getline(&s[count-1], NULL, stdin));

    for(int i = 0; i < count; i++) printf(s[i]);
}
Improve this answer

Comments

0

Why do you use malloc in the first place. malloc is very very dangerous!! Just use s[1000][1000] and pass &s[0][0] for the first line, &s[1][0] for the second line etc.

For printing printf("%s \n", s[i]); where i is the line you want.

Improve this answer

8 Comments

And using fixed size buffers on the stack is not dangerous?!? Are you an NSA-employee trying to coax people into writing insecure code?
it will be outside of the main function obviously!
That does not make fixed size buffer on the stack any better. Whatever size you choose for your buffer, you will at some point exceed it, and then you will have trouble finding out why your program is crashing. And you will limit your users to not supplying longer lines. Really, 99% of all fixed buffer sizes I have seen are bugs in disguise.
And using realloc in a loop is a great thing obviously!
Why do you use malloc in the first place.. malloc is very very dangerous!! I could not make up my mind whether to down vote you, or up vote you. The down vote for the silliness of your statement. The upvote for the entertainment value of it. So, +1 (and -1).
|

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.