Reading arbitrarily long lines from a file that is redirected as input

Question

So I have a file with a list of dates that are to be redirected as input to stdin and what the program does is it searches for valid dates, appends them to the output file and then appends all the dates in the input file to the output file. In other words, at the end of the program there would be a list of all the valid dates in the output file and list of all the dates that were in the input file. An important command line argument is the number of entries to search for. 0 here is the default case where we look through the entire input file. Any value greater than 0 represent the number of entries we have to look at. If we find one that is invalid, we skip it.

Command Example: ./program < input.txt 0> output.txt

Here are all the relevant functions.

getDates Function

char ** getDates(char **lines, size_t *numOfLines, size_t *lineMax, int numOfEntries)
{
 char *strDate = NULL;
 int allEntries = FALSE, errorStatus = FALSE; // errorStatus is so I can report memory failures.
 if(numOfEntries == 0) 
 {
 allEntries = TRUE; // A flag for the default case zero.
 }
 while((strDate = readLine(stdin, &errorStatus)) != NULL)
 {
 if(numOfEntries >= 0 || allEntries)
 {
 if(checkDate(strDate))
 {
 fputs(strDate, stdout);
 }
 numOfEntries--;
 }
 /* 
 Keep a record of how many lines we have process. 
 We will need this variable when we decide to print 
 everything that we have in the variable lines. 
 */
 lines[*numOfLines] = strDate;
 (*numOfLines)++; 
 if((*numOfLines) == (*lineMax))
 {
 errorStatus = expand(lines, lineMax); // try to expand.
 if(errorStatus == MEMFAILURE)
 {
 fprintf (stderr,"Error, couldn't allocate any more memory.");
 return NULL;
 }
 }
 }
 if(errorStatus == MEMFAILURE)
 {
 fprintf (stderr,"Error, couldn't allocate any more memory.");
 return NULL;
 }
 if(ferror(stdin))
 {
 fprintf(stderr,"Error, couldn't completely process the entire file.\n");
 return NULL;
 }
 if(strDate != NULL)
 {
 free(strDate);
 }
 return lines;
}

readLine Function

char *readLine(FILE *stream, int *errorStatus)
{
 size_t size = 0;
 char *buffer = NULL;
 char *newLineFound = NULL;
 do
 {
 char *temp = realloc(buffer, size + BUFSIZ); // BUFSIZE is a define constant in the stdio library.
 if(!temp)
 {
 free(buffer);
 *errorStatus = MEMFAILURE; // an enum constant to record our error.
 return NULL;
 }
 buffer = temp;
 // One small problem. If there is no new line at the end of the file. it won't add whatever contents is there.
 if(!fgets(buffer + size, BUFSIZ, stream))
 {
 free(buffer);
 return NULL;
 }
 newLineFound = strchr(buffer + size, '\n');
 }
 while(!newLineFound && (size += BUFSIZ - 1)); // this is to simply update size at the same time that we don't find a newline.
 return buffer;
}

expand Function

int expand(char **lines, size_t *lineMax)
{
 char **tmp = realloc (lines, (*lineMax) * 2 * sizeof(*lines));
 if(!tmp)
 {
 return MEMFAILURE;
 }
 lines = tmp;
 (*lineMax) *= 2; // Update the size
 return SUCCESS;
}

• What can I do to reduce my code as I feel that it is getting too bloated.
• What did I forget to do, what didn't I do right and what did I do that was unnecessary.
• Performance tips are welcome.

Answer 1

What can I do to reduce my code as I feel that it is getting too bloated.

1. No need for test. Just call free();. It handles free(NULL);

   // if(strDate != NULL) {
   // free(strDate);
   // }
   free(strDate);
   

2. Minor: Style: I found the vertical spacing excessive creating challenges seeing the overall flow. YMMV

What did I forget to do, what didn't I do right and what did I do that was unnecessary.

1. Certain functional error, lines is assigned a value that is not used. Need to pass a "reference".

   int expand(char **lines, size_t *lineMax) {
    char **tmp = realloc (...);
    ...
    lines = tmp;
    ... 
    return SUCCESS;
   }
   

2. Functional error (possible part of your "One small problem." comment). If fgets() returns NULL, code should only dispose of the buffer if this is the first fgets() in readLine(). Or: if the reason for NULL is due to ferror() and not feof().

3. Functional corner case: strchr(buffer searches the buffer and can get fooled by fgets() reading a null charter and then a end-of-line such as "0円\n". No true way to solve this and still use fgets().

    newLineFound = strchr(buffer + size, '\n');
   }
   while(!newLineFound && (size += BUFSIZ - 1)); 
   

4. Minor: Format to the presentation width.

   // avoid
   int allEntries = FALSE, errorStatus = FALSE; // errorStatus is so I can report memory failures.
   // better
   // errorStatus is so I can report memory failures.
   int allEntries = FALSE, errorStatus = FALSE; 
   

5. Use bool. It is standard and well defined.

   #include <stdbool.h>
   // int allEntries = FALSE, errorStatus = FALSE;
   bool allEntries = false, errorStatus = false;
   

6. Not all stderr are unbuffered. Suggest appending '\n' to your print to insure timely display. Note either call below may make the same code with smart compilers. 2nd has the advantage of no problems with '%'

   // fprintf (stderr,"Error, couldn't allocate any more memory.");
   fprintf(stderr,"Error, couldn't allocate any more memory.\n");
   // or 
   fputs("Error, couldn't allocate any more memory.\n", stderr);
   

7. Confusing code: simplification suggestion

   // char **tmp = realloc (lines, (*lineMax) * 2 * sizeof(*lines));
   size_t newsize = *lineMax * 2;
   void *tmp = realloc (lines, sizeof(*lines) * newsize);
   ....
   *lineMax = newsize;
   

8. Lost memory

   char **tmp = realloc (lines, (*lineMax) * 2 * sizeof(*lines));
   if(!tmp) {
    // Add
    free(lines);
    return MEMFAILURE;
   }
   

9. Functional corner case: If *lineMax could ever be 0. Returning NULL is OK when the size of 0 and not a MEMFAILURE.

   char **tmp = realloc (lines, (*lineMax) * 2 * sizeof(*lines));
   // if(!tmp) {
   if(!tmp && *lineMax) {
    return MEMFAILURE;
   }
   

10. Functional: To be more resilient, check size before using the array. getDates() does not control input of the count/size.

    // move here
    if((*numOfLines) >= (*lineMax)) ...
    lines[*numOfLines] = strDate;
    (*numOfLines)++; 
    // if((*numOfLines) == (*lineMax)) ...
    

Performance tips are welcome.

1. I/O is often a candidate for speed improvements. For this code, if there is no other calls to stdin, I would use fread() and then process the lines myself, reading large blocks.

2. On systems that use '\n' translation, improvement can be had by reading in binary mode.

3. When a file ends without a final '\n', code is probably faster appending one, insuring the rest of code does not have to check for that expectation.

GTG

• Luis Averhoff: In number 8, why are you freeing lines and not temp? In number 7, why did you change temp to single pointer instead of a double pointer?
• chux: @Luis Averhoff #8: free(temp) is the same as the uneeded free(NULL);. free(lines) because the code no longer has access to lines. IAC, expand()1 needs re-work per #1.
• chux: @Luis Averhoff #7 tmp is a void * - the universal "we don't care about the type" pointer. It is just a code simplification. - minor bloat reduction.
• Luis Averhoff: Well I could make line a triple pointer(you said I need to pass a reference) but that would degrade code readability. So I guess I could just make expand return NULL if realloc fails or lines if it succeeds.
• Luis Averhoff: For number 9, how can *lineMax ever be zero if I initially set it some value at the start of the program.