Basic program design, passing pointer to a function

This is a discussion on Basic program design, passing pointer to a function within the C Programming forums, part of the General Programming Boards category; Hi, The programs below try to read a file and print its contents to a block of 72 chars wide. ...

  1. #1
    Registered User
    Join Date
    Mar 2008
    Posts
    44

    Basic program design, passing pointer to a function

    Hi,
    The programs below try to read a file and print its contents to a block of 72 chars wide. The point of the programs is for me to learn not to cram everything into main(). What I'd like it to do is the following in pseudo code:
    Code:
    /*
    func1()
      open text file
    
    func2()
      make '\0' terminated line
      return pointer of line to main
    
    main()
      func1()
      loop func2 until EOF
     */
    The first kludge I came up with is this:
    Code:
    // readfile1.c
    
    #include <stdio.h>
    #include <stdlib.h>
    #include <string.h>
    #include <errno.h>
    
    static FILE *open_file(char *file, char *mode)
    {
    	FILE *fp = fopen(file, mode);
    	if (fp == NULL) {
    		perror("fopen: ");
    		exit(EXIT_FAILURE);
    	}
    	return fp;
    }
    
    char *read_line(int line_length, FILE *txt)
    {
    	int i, character;
    	char *line;
    	line = (char *)malloc(line_length);
    	FILE *text;
    
    	for (i = 0; i < (line_length - 1); i++) {
    		if ((character = fgetc(text)) == EOF) {
    			perror("fgetc: ");
    			exit(EXIT_SUCCESS);
    		}
    		line[i] = character;
    	}
    	line[line_length - 1] = '\0';
    
    	return(line);
    }
    
    int main(int argc, char *argv[])
    {
    	int length = 73;
    	FILE *open;
    	open = open_file(argv[1], "r");
    
    	int i;
    	// This test loop is a place holder for a loop_until_EOF.
    	// It segfaults at i = 1.
    	for (i = 0; i < 4; i++) printf("%s\n", read_line(length, open));
    
    	return(EXIT_SUCCESS);
    }
    Code:
    heras@vhbh:~/code/c$ ./read1 ~/code/text
    
    Open text file for reading. The stream is positioned at the beginning
    Segmentation fault (core dumped)
    The malloc in the readline() funtion is probably a bad idea because it would require a seperate line = (char *)malloc(line_length); buffer for each line that is read from the source file. And to free() them (I'm not at that point yet) might be cumbersome. Is any of this true?
    Then I tried to move the malloc to main() and pass line_pointer to read_line() hoping read_line() might write to it. This is not working at all:
    Code:
    //readfile2.c
    
    #include <stdio.h>
    #include <stdlib.h>
    #include <string.h>
    #include <errno.h>
    
    static FILE *open_file(char *, char *);
    char *read_line(int, FILE *, char *);
    
    int main(int argc, char *argv[])
    {
    	FILE *open;
    	open = open_file(argv[1], "r");
    
    	int i;
    	int length = 73;
    	char *line_pointer;
    	line_pointer = (char *)malloc(length);
    	for (i = 0; i < 4; i++) {
    		printf("%s\n", read_line(length, open, line_pointer));
    	}
    	return(EXIT_SUCCESS);
    }
    
    static FILE *open_file(char *file, char *mode)
    {
    	FILE *fp = fopen(file, mode);
    	if (fp == NULL) {
    		perror("fopen");
    		exit(EXIT_FAILURE);
    	}
    	return(fp);
    }
    
    char *read_line(int line_length, FILE *txt, char *lp)
    {
    	int i;
    	char *character = lp;
    	FILE *text;
    
    	for (i = 0; i < (line_length - 1); i++) {
    		*character++ = fgetc(text);
    	}
    	return(lp);
    }
    Code:
    heras@vhbh:~/code/c$ ./read2 ~/code/text
    Segmentation fault (core dumped)
    Code:
    (gdb) b main
    Breakpoint 1 at 0x80484a5: file readfile2.c, line 12.
    (gdb) r ~/code/text
    Starting program: /home/heras/code/c/read2 ~/code/text
    
    Breakpoint 1, main (argc=2, argv=0xbfcea004) at readfile2.c:12
    12              open = open_file(argv[1], "r");
    (gdb) n
    15              int length = 73;
    (gdb)
    17              line_pointer = (char *)malloc(length);
    (gdb)
    18              for (i = 0; i < 4; i++) {
    (gdb)
    19                      printf("%s\n", read_line(length, open, line_pointer));
    (gdb)
    
    Program received signal SIGSEGV, Segmentation fault.
    0xb7edb46d in getc () from /lib/tls/i686/cmov/libc.so.6
    (gdb)
    Single stepping until exit from function getc,
    which has no line number information.
    
    Program terminated with signal SIGSEGV, Segmentation fault.
    The program no longer exists.
    I have no clear question, by now I don't know what I'm doing anymore. Any guidance or hints on how to construct such a program would be greatly appreciated.

    Two more minor / unimportant issues:
    - printing a line is off by 2. It prints 70 characters but I don't see why
    - if I get this to work at all, to reuse it in a socket/network program with as little modification as possible

    Thanks,
    heras

  2. #2
    Kernel hacker
    Join Date
    Jul 2007
    Location
    Farncombe, Surrey, England
    Posts
    15,677
    Code:
    char *read_line(int line_length, FILE *txt)
    ...
    	FILE *text;
    
    	for (i = 0; i < (line_length - 1); i++) {
    		if ((character = fgetc(text)) == EOF) {
    			perror("fgetc: ");
    You are using the uninitialized local "text" instead of the argument txt - so you are using a "rubbish" file descriptor, and that causes the seg fault.

    --
    Mats
    Compilers can produce warnings - make the compiler programmers happy: Use them!
    Please don't PM me for help - and no, I don't do help over instant messengers.

  3. #3
    uint64_t...think positive xuftugulus's Avatar
    Join Date
    Feb 2008
    Location
    Pacem
    Posts
    355
    Code:
    char *read_line(int line_length, FILE *txt, char *lp)
    {
    	int i;
    	char *character = lp;
    	FILE *text;
    
    	for (i = 0; i < (line_length - 1); i++) {
    		*character++ = fgetc(text);
    	}
            *character = '\0';  /* A good idea to terminate the string */
    	return(lp);
    }
    You would be much better to store what fgetc returns to an int.
    That way you can test whether fgetc returned EOF, and break out of the for loop.
    Also for starters, don't use assignment and pointer increment at the same time.
    Break the statements to verify your logic of code flow.
    My point is that your pointer named 'character' should be assigned after being incremented,
    if there was a character read.
    Code:
    ...
        goto johny_walker_red_label;
    johny_walker_blue_label: exit(-149$);
    johny_walker_red_label : exit( -22$);
    A typical example of ...cheap programming practices.

  4. #4
    Registered User
    Join Date
    Mar 2008
    Posts
    44

    Thanks matsp! That was an easier fix than I thought. xuftugulus, thanks for catching those bugs, I'll clean up the loop with proper checking.

    The off by 2 thing turns out to be related to the source file (perhaps a carriage return), it prints 72 char lines just fine.

  5. #5
    Registered User
    Join Date
    Mar 2008
    Posts
    44
    Yay! It now does what I want, sort of. I have some questions though.

    - gcc -Wall doesn't care whether read_line() is int or char. I thought EOF is int, no?
    - Is there a point in freeing memory just before exiting?
    - Is there a nicer way to do the end = EOF; return (end); thingy?
    - Any comments on beginner badness are most welcome.

    Thanks,
    heras

    Code:
    /*
            readfile2.3.c
    */
    
    #include <stdio.h>
    #include <stdlib.h>
    #include <string.h>
    #include <errno.h>
    #include <ctype.h>
    
    static FILE *open_file(char *, char *);
    int read_line(int, FILE *, char *);
    
    int main(int argc, char *argv[])
    {
        if (argc != 2) {
            fprintf(stderr, "%s usage: <address>\n", argv[0]);
            exit (EXIT_FAILURE);
        }
    
        FILE *open;
        open = open_file(argv[1], "r");
    
        int line_length = 73;
        char *line_pointer;
        line_pointer = (char *)malloc(line_length);
        while (read_line(line_length, open, line_pointer) != EOF) {
            printf("%s\n", line_pointer); //FIXME: does not print last line
        }
        free(line_pointer);
        return (EXIT_SUCCESS);
    }
    
    static FILE *open_file(char *file, char *mode)
    {
        FILE *fp = fopen(file, mode);
        if (fp == NULL) {
            perror("fopen");
            exit(EXIT_FAILURE);
        }
        return (fp);
    }
    
    int read_line(int line_length, FILE *file, char *line_pointer)
    {
        int i = 0, end = 0, c;
        while ((c = fgetc(file) ) != EOF) {
            if ((isalnum(c) != 0 || ispunct(c) != 0 || c == ' ')) {
                *line_pointer = c;
                line_pointer++;
            }
            if (i == (line_length - 2)) {
                break;
            }
            i++;
        }
        *line_pointer = '\0';
        if (c == EOF) {
            end = EOF;
        }
        return (end);
    }
    Code:
    heras@vhbh:~/code/c$ ./read ~/code/text
     Open text file for reading. The stream is positioned at the beginning
    of the file. r+ Open for reading and writing. The stream is positioned
     at the beginning of the file. w Truncate file to zero length or creat
    e text file for writing. The stream is positioned at the beginning of th
    e file. w+ Open for reading and writing. The file is created if it doe
    s not exist, otherwise it is truncated. The stream is positioned at the
    beginning of the file. a Open for appending (writing at end of file).
    The file is created if it does not exist. The stream is positioned at th
    e end of the file. a+ Open for reading and appending (writing at end o
    f file). The file is created if it does not exist. The initial file posi
    tion for reading is at the beginning of the file, but output is always a
    heras@vhbh:~/code/c$ Yay! :D

  6. #6
    Kernel hacker
    Join Date
    Jul 2007
    Location
    Farncombe, Surrey, England
    Posts
    15,677
    > - gcc -Wall doesn't care whether read_line() is int or char. I thought EOF is int, no?

    EOF is defined as -1, which in signed char is indistinguishable from the char of all ones (255 == -1). Character number 255 is not commonly used, but if you read a binary file it may well occur.

    > - Is there a point in freeing memory just before exiting?
    Yes, neatness and completeness.

    > - Is there a nicer way to do the end = EOF; return (end); thingy?
    See below.

    > - Any comments on beginner badness are most welcome.
    See below


    There is a function called "open()" [in Linux at the very least - recent MS libraries call this _open()]. So don't call your FILE * variable open - you'll get into trouble sooner or later if you do.

    You do not check if fopen() is successfull.

    I would re-order the arguments to read_line(), such that you pass linelenght and linebuffer next to each other, with the FILE * argument either at the front or the back of the list.

    You can do the return statement in several different ways. Here's a few options:
    Code:
    return (c == EOF)?EOF:0;
    // or simpler, since you are checking for exactly EOF:
    return c; 
    // or:
    if (c == EOF) return EOF; 
    else return 0;
    The above solutions replace the "end" variable and any code related to setting the end variable.

    Why is open_file static, but read_line isn't?

    --
    Mats
    Compilers can produce warnings - make the compiler programmers happy: Use them!
    Please don't PM me for help - and no, I don't do help over instant messengers.

  7. #7
    Registered User
    Join Date
    Mar 2008
    Posts
    44
    Quote Originally Posted by matsp View Post
    You do not check if fopen() is successfull.
    So checking if (fp == NULL) is not enough? When I enter a non-existent file name, the program quits with "fopen: No such file or directory". Perhaps this would be better? (I don't know if this compiles but something along these lines):
    Code:
    static FILE *open_file(char *file, char *mode)
    {
        if ((FILE *fp = fopen(file, mode)) == -1) {
            perror("fopen");
            exit(EXIT_FAILURE);
        }
        return (fp);
    }
    return c;
    Hehe! I'm definitely not the sharpest tool in the box.

    Why is open_file static, but read_line isn't?
    I'm going to think about that because I actually don't know.

    Thanks a lot for your feedback.
    Cheers,
    heras

  8. #8
    C++まいる!Cをこわせ! Elysia's Avatar
    Join Date
    Oct 2007
    Posts
    22,170
    Note that simply calling exit from a function is discouraged. The function should generally return success or failure and the caller should take appropriate action.
    Quote Originally Posted by Adak View Post
    io.h certainly IS included in some modern compilers. It is no longer part of the standard for C, but it is nevertheless, included in the very latest Pelles C versions.
    Quote Originally Posted by Salem View Post
    You mean it's included as a crutch to help ancient programmers limp along without them having to relearn too much.

    Outside of your DOS world, your header file is meaningless.
    For information on how to enable C++11 on your compiler, look here.
    よく聞くがいい!私は天才だからね! ^_^

  9. #9
    Kernel hacker
    Join Date
    Jul 2007
    Location
    Farncombe, Surrey, England
    Posts
    15,677
    Quote Originally Posted by heras View Post
    So checking if (fp == NULL) is not enough? When I enter a non-existent file name, the program quits with "fopen: No such file or directory". Perhaps this would be better? (I don't know if this compiles but something along these lines):
    Code:
    static FILE *open_file(char *file, char *mode)
    {
        if ((FILE *fp = fopen(file, mode)) == -1) {
            perror("fopen");
            exit(EXIT_FAILURE);
        }
        return (fp);
    }
    Sorry, I wasn't paying attention - that's fine as it is. Although it is often better to return failure to the calling code if it's not a completely catastrophic failure.

    Re: static, Do you understand what static actually does?

    --
    Mats
    Compilers can produce warnings - make the compiler programmers happy: Use them!
    Please don't PM me for help - and no, I don't do help over instant messengers.

  10. #10
    Registered User
    Join Date
    Mar 2008
    Posts
    44
    The function should generally return success or failure and the caller should take appropriate action.
    Although it is often better to return failure to the calling code if it's not a completely catastrophic failure.
    Thanks, I'll try to move that stuff to main().

    Do you understand what static actually does?
    I think it makes sure that the source of whatever kind cannot be altered. Or was that const ... But I haven't yet rtfm of either static or const.

    edit:
    In computer programming, static variables data value persists for the life of the running process and typically have a broader scope than other variables. Their values may be set at run-time or may change subtly during program execution.
    Reads somewhat like global variables. But anywho, I'm of to the documentation.
    Last edited by heras; 04-01-2008 at 07:00 AM.

  11. #11
    Kernel hacker
    Join Date
    Jul 2007
    Location
    Farncombe, Surrey, England
    Posts
    15,677
    static on variables make them act like globals except with more limited scope (if they are global in the file, they will only be visible within the file, if the static is in a function then the variable is only visible in the function - so no other function can "mess" with the variable).

    But for functions, it has the same meaning as static on a file-global variable: it hides the function from other modules in the project. Since this is a single-file-project, it makes little difference here - I was just curious as to why you decided to make one static and the other one not.

    --
    Mats
    Compilers can produce warnings - make the compiler programmers happy: Use them!
    Please don't PM me for help - and no, I don't do help over instant messengers.

  12. #12
    Registered User
    Join Date
    Mar 2008
    Posts
    44
    Quote Originally Posted by matsp View Post
    Since this is a single-file-project, it makes little difference here - I was just curious as to why you decided to make one static and the other one not.
    Thanks for the explanation, I think I understand
    Actually, the next thing I wanted to try is putting the functions in separate files, so by then it may be more relevant to consider this.

  13. #13
    Registered User
    Join Date
    Mar 2008
    Posts
    44
    And after asking twice, I notice that I still haven't answered your question, sorry:
    I was just curious as to why you decided to make one static and the other one not.
    My implementation comes from this FAQ without fully understanding it.
    I think I understand
    Turns out I'm wrong. The below code produces an "interesting" compiler output:
    Code:
    heras@vhbh:~/code/c/readfile$ gcc -Wall readfile.c open_file.c read_line.c -o read
    open_file.h:1: warning: ‘open_file’ used but never defined
    open_file.c:7: warning: ‘open_file’ defined but not used
    /tmp/ccMsUG3J.o: In function `main':
    readfile.c:(.text+0x61): undefined reference to `open_file'
    collect2: ld returned 1 exit status
    Code:
    /*
    		readfile2.3.c
    */
    
    #include <stdio.h>
    #include <stdlib.h>
    #include "open_file.h"
    #include "read_line.h"
    
    int main(int argc, char *argv[])
    {
    	if (argc != 2) {
    		fprintf(stderr, "%s usage: <address>\n", argv[0]);
    		exit (EXIT_FAILURE);
    	}
    
    	FILE *open_fd;
    	open_fd = open_file(argv[1], "r");
    
    	int line_length = 73;
    	char *line_pointer;
    	line_pointer = (char *)malloc(line_length);
    	while (read_line(open_fd, line_length, line_pointer) != EOF) {
    		printf("%s\n", line_pointer); //FIXME: does not print last line
    	}
    	free(line_pointer);
    	return (EXIT_SUCCESS);
    }
    open_file.h
    Code:
    static FILE *open_file(char *, char *);
    open_file.c
    Code:
    #include <stdio.h>
    #include <stdlib.h>
    #include <errno.h>
    #include "open_file.h"
    
    static FILE *open_file(char *file, char *mode)
    {
    	FILE *fp = fopen(file, mode);
    	if (fp == NULL) {
    		perror("fopen");
    		exit (EXIT_FAILURE); //FIXME: return failure
    	}
    	return (fp);
    }
    read_line.h
    Code:
    int read_line(FILE *, int, char *);
    read_line.c
    Code:
    #include <stdio.h>
    #include <ctype.h>
    #include "read_line.h"
    
    //FIXME: static int read_line() ?
    int read_line(FILE *file, int line_length, char *line_pointer)
    {
    	int i = 0, c;
    	while ((c = fgetc(file) ) != EOF) {
    		if (isalnum(c) != 0 || ispunct(c) != 0 || c == ' ') {
    			*line_pointer = c;
    			line_pointer++;
    		}
    		if (i == (line_length - 2)) {
    			break;
    		}
    		i++;
    	}
    	*line_pointer = '\0';
    
    	return (c);
    }
    Removing static from both open_file.h and open_file.c makes the "interesting" part go away.
    Note1 that some of the badness has not been fixed, but it will be.
    Note2 that matsp rules (together with all the other helpfull folks in these parts).

    ps:I don't have a question about this code, it's just a follow-up.

  14. #14
    Kernel hacker
    Join Date
    Jul 2007
    Location
    Farncombe, Surrey, England
    Posts
    15,677
    That is EXACTLY what the expected effect of static gives - it hides the function from other compilation units (that is, other .c files).

    --
    Mats
    Compilers can produce warnings - make the compiler programmers happy: Use them!
    Please don't PM me for help - and no, I don't do help over instant messengers.

  15. #15
    C++まいる!Cをこわせ! Elysia's Avatar
    Join Date
    Oct 2007
    Posts
    22,170
    See how the linker complained it couldn't find the function? Indeed, that's because it was hidden. So you get a linking error. And if you remove it... then it works.
    Quote Originally Posted by Adak View Post
    io.h certainly IS included in some modern compilers. It is no longer part of the standard for C, but it is nevertheless, included in the very latest Pelles C versions.
    Quote Originally Posted by Salem View Post
    You mean it's included as a crutch to help ancient programmers limp along without them having to relearn too much.

    Outside of your DOS world, your header file is meaningless.
    For information on how to enable C++11 on your compiler, look here.
    よく聞くがいい!私は天才だからね! ^_^

Popular pages Recent additions subscribe to a feed

Similar Threads

  1. Getting an error with OpenGL: collect2: ld returned 1 exit status
    By Lorgon Jortle in forum C++ Programming
    Replies: 6
    Last Post: 05-08-2009, 08:18 PM
  2. In over my head
    By Shelnutt2 in forum C Programming
    Replies: 1
    Last Post: 07-08-2008, 06:54 PM
  3. Undefined Reference Compiling Error
    By AlakaAlaki in forum C++ Programming
    Replies: 1
    Last Post: 06-27-2008, 11:45 AM
  4. Replies: 9
    Last Post: 12-25-2007, 04:01 AM
  5. Dikumud
    By maxorator in forum C++ Programming
    Replies: 1
    Last Post: 10-01-2005, 06:39 AM

1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21