2

I'm making a function char** read_from_file(char* fname, int * size) that reads all the words from a file fname and returns them as char**. My file only has 5 words, there is only one word per line. I then have another function print_strings(char** words, int num_words) that prints the strings.

I'm having 3 problems:

  1. When I am comparing the index to < *size I get "comparison between pointer and integer"

  2. I can't store the words in the **words

  3. I'm not sure how to return all the words.

This is my code:

void test_sort(char* fname){
    int i;   
    int num_words;
    char** words = read_from_file(fname, &num_words);

    printf("\n ORIGINAL data:\n");
    print_strings(words, num_words);
}

In Main:

int main(){    

    // test sorting array of string by string length
    test_sort("data.txt");
}

Reading Function

char** read_from_file(char* fname, int * size)  { 

    char** words = (char **)malloc(N_MAX);
    FILE *ifp;
    ifp = fopen(fname, "r");
    if(ifp == NULL){
        fprintf(stderr, "Can't open file\n");
        exit(1);
    }

    int index;

    while (!feof(ifp)){
        for(index = 0; index < size; index++)
        {
            fscanf(ifp,"%s", words[index]);
        }
    }
    fclose(ifp);

    return words;
}
4
  • Do not cast the result of malloc & friends and void * in general in C. Nov 22, 2015 at 3:08
  • Do not use feof() here. Test the result of fscanf() instead. Who or what text suggested feof()? Nov 22, 2015 at 5:12
  • char** words = (char **)malloc(N_MAX); should be char** words = malloc(N_MAX * sizeof *words); you must then allocate for each line (word). char buf[64] = {0}; size_t index = 0; while (fgets (buf, 64, ifp)) { words[index] = malloc (65); strncpy (word[index++], buf, 64); } (presumes 1 word per-line, words all < 64 char, which given the longest in the abridged dictionary is 28 chars is a safe presumption). Nov 22, 2015 at 6:27
  • @chux It's something I had seen online.
    – David
    Nov 22, 2015 at 6:58

1 Answer 1

3

When you allocate an array of pointer-to-pointer-to-char (e.g. char **words;), you must allocate memory for the array of pointers:

char **words = malloc (N_MAX * sizeof *words);

as well as each array of characters (or string) pointed to by each pointer:

words[index] = malloc ((N_MAX + 1) * sizeof **words);

or simply:

words[index] = malloc (N_MAX + 1);

or when allocating memory for a null-terminated string, a shortcut with strdup that both allocates sufficient memory to hold the string and copies the string (including the null-terminating character) to the new block of memory, returning a pointer to the new block:

words[index] = strdup (buf);

A short example of your intended functions could be as follows (note, index is passed as a pointer below, so it must be deferenced to obtain its value *index):

#include <stdio.h>
#include <stdlib.h>
#include <string.h>

#define N_MAX 10
#define C_MAX 64

void test_sort(char* fname);
char** read_from_file(char* fname, size_t *index);

int main (void) {    

    test_sort ("data.txt");
    return 0;
}

void test_sort (char* fname)
{
    size_t i = 0;  
    size_t num_words = 0;

    char **words = read_from_file (fname, &num_words);

    printf("\n ORIGINAL data:\n\n");
    // print_strings(words, num_words);
    for (i = 0; i < num_words; i++)
        printf ("   words[%2zu] : %s\n", i, words[i]);
    putchar ('\n');

    /* free allocated memory */
    for (i = 0; i < num_words; i++)
        free (words[i]);
    free (words);
}

char** read_from_file (char* fname, size_t *index)
{ 
    char **words = malloc (N_MAX * sizeof *words);
    char buf[C_MAX] = {0};
    FILE *ifp = fopen (fname, "r");

    if (ifp == NULL){
        fprintf (stderr, "Can't open file\n");
        exit(1);
    }

    *index = 0;
    while (fgets (buf, C_MAX, ifp))
    {
        char *p = buf;  /* strip trailing newline/carriage return */
        size_t len = strlen (p);
        while (len && (p[len-1] == '\r' || p[len-1] == '\n'))
            p[--len] = 0;

        /* strdup allocates and copies buf */
        words[(*index)++] = strdup (buf);

        if (*index == N_MAX) {
            fprintf (stderr, "warning: N_MAX words read.\n");
            break;
        }
    }
    fclose(ifp);

    return words;
}

Input

$ cat data.txt
A quick
brown fox
jumps over
the lazy
dog.

Output

$ ./bin/read5str

 ORIGINAL data:

   words[ 0] : A quick
   words[ 1] : brown fox
   words[ 2] : jumps over
   words[ 3] : the lazy
   words[ 4] : dog.

Memory Error Check

In any code your write that dynamically allocates memory, you have 2 responsibilites regarding any block of memory allocated: (1) always preserves a pointer to the starting address for the block of memory so, (2) it can be freed when it is no longer needed. It is imperative that you use a memory error checking program to insure you haven't written beyond/outside your allocated block of memory and to confirm that you have freed all the memory you have allocated. For Linux valgrind is the normal choice. There are so many subtle ways to misuse a block of memory that can cause real problems, there is no excuse not to do it. There are similar memory checkers for every platform. They are all simple to use. Just run your program through it.

$ valgrind ./bin/read5str
==5507== Memcheck, a memory error detector
==5507== Copyright (C) 2002-2012, and GNU GPL'd, by Julian Seward et al.
==5507== Using Valgrind-3.8.1 and LibVEX; rerun with -h for copyright info
==5507== Command: ./bin/read5str
==5507==

 ORIGINAL data:

   words[ 0] : A quick
   words[ 1] : brown fox
   words[ 2] : jumps over
   words[ 3] : the lazy
   words[ 4] : dog.

==5507==
==5507== HEAP SUMMARY:
==5507==     in use at exit: 0 bytes in 0 blocks
==5507==   total heap usage: 7 allocs, 7 frees, 691 bytes allocated
==5507==
==5507== All heap blocks were freed -- no leaks are possible
==5507==
==5507== For counts of detected and suppressed errors, rerun with: -v
==5507== ERROR SUMMARY: 0 errors from 0 contexts (suppressed: 2 from 2)

The salient parts above are that there were 7 allocs, 7 frees and All heap blocks were freed. Further ERROR SUMMARY: 0 errors from 0 contexts. You should receive similar output every time. Let me know if you have additional questions.

3
  • Surely you meant words[index] = malloc ((N_MAX + 1) * sizeof **words); () added. I would have used words[index] = malloc (sizeof *words[index] * (N_MAX + 1)); to preserve the idiom of p = malloc(sizeof *p * N). Nov 22, 2015 at 17:59
  • 1
    I tip my hat to your sir... Again. That is, of course, what I meant to write, but there seems to be some minor disconnect between my brain and finger tips... Early onset dementia? Senility? Hard to tell. Fixed. Nov 23, 2015 at 5:49
  • How does one know they have senility? Nov 23, 2015 at 5:52

Your Answer

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

Not the answer you're looking for? Browse other questions tagged or ask your own question.