10
\$\begingroup\$

After a successful fgets(buffer, ...), it is often desirable to trim a potential End-of-Line '\n'.

Of the following 2 methods, are there any shortcomings?

#include <stdio.h>

char buffer[100];

// Method 1
while (fgets(buffer, sizeof buffer, stdin) != NULL) {
  size_t len = strlen(buffer);
  if (len > 0 && buffer[len - 1] == '\n')
    buffer[--len] = '\0';

  // use buffer
  printf("\"%s\"\n", buffer);
}

// Method 2
while (fgets(buffer, sizeof buffer, stdin) != NULL) {
  strtok(buffer, "\n");

  // use buffer
  printf("\"%s\"\n", buffer);
}

Notes:

  1. When reading text files from alternate file systems, strtok(buffer, "\r\n") looks useful.
  2. strtok() may incur an issue with another strtok() sequence.
\$\endgroup\$
  • \$\begingroup\$ Could you please make this into a short runnable program with examples of how you have tested it and expect it to run? And could you expand what you mean by "incur an issue with another strtok() sequence"? \$\endgroup\$ – syb0rg Oct 22 '14 at 22:23
  • \$\begingroup\$ @syb0rg strtok() is not re-entrant. There is a strtok_r() variant that is re-entrant. \$\endgroup\$ – 200_success Oct 22 '14 at 22:40
  • \$\begingroup\$ @200_success I know, I thought that is what he meant but just wanted to confirm. Note that strtok_r is provided by the POSIX standard, and that strtok_s is the C11 standard version. \$\endgroup\$ – syb0rg Oct 22 '14 at 22:44
  • \$\begingroup\$ @syb0rg 1) In re: "incur an issue with another strtok() sequence": If code was in the middle of calling a series of strtok() to perform some other tokenizing, then this use of strtok() would fouls things up. That does not seem likely, but is a down side. 2) Thanks 1K 3) Did not know about strtok_r() 4) strtok_s() in in the normative "Bounds-checking interfaces" section of C11. \$\endgroup\$ – chux - Reinstate Monica Oct 23 '14 at 2:56
8
\$\begingroup\$

For what it is worth, using strlen is much more efficient than using strtok. I did some tests (omitting the file access) using strlen, strchr, strtok, strstr, strpbrk and strcspn:

It surprised me that to execute this neat looking thing:

s = strchr(s, '\n');
if (s) {
    *s = '\0';
}

takes nearly 50% longer than this ugly looking sucker:

size_t len = strlen(s);
if (len && (s[len-1] == '\n')) {
    s[len-1] = '\0';
}

and the beautiful

strtok(s, "\n\r");

and

strsep(&s, "\n\r");

are 10 to 20 times slower. The latter two do of course look for \r as well as \n. But even adding \r, the strlen approach is still quicker than the strchr and still quicker then strtok and strsep by 10 to 20 times:

size_t len = strlen(s);
if (len && ((s[len-1] == '\n') || (s[len-1] == '\r'))) {
    s[len-1] = '\0';
}

Note that I wasn't exhaustive with string variations. In fact I didn't even break a sweat. And of course this is all quite irrelevant as the stdio call will dominate in your target code :-)

Note also from the comments below that if the first character of the search string (s in the examples above) is one of the characters in the pattern string, strtok does nothing (thanks to @chux).

\$\endgroup\$
  • \$\begingroup\$ Recently found out strtok(s, "\n\r"); does not always trim the '\n' from s. When s == "\n". The '\n' remains. \$\endgroup\$ – chux - Reinstate Monica Jan 1 '15 at 9:05
  • 1
    \$\begingroup\$ @chux I tried it on a Mac and you are right - which system did you use? I'm shocked - it is rather like finding out that an old friend has a dark secret. And it is not specific to \n - if any char in the search pattern appears as the first char in s, strtok does nothing. My man page doesn't describe this behaviour. Another reason not to use strtok - strsep works as expected. \$\endgroup\$ – William Morris Jan 1 '15 at 14:17
  • \$\begingroup\$ Might want to add this about strtok() as relates to "are there any shortcomings?" Looked into char *ptr = buffer; strsep(ptr, "\n") and did not find any functional shortcomings, other than it is non-standard. \$\endgroup\$ – chux - Reinstate Monica Jan 1 '15 at 16:37
4
\$\begingroup\$

For completeness, I tried a few alternative ways of doing the file access. I tried your fgets and the suggested alternatives from other answers.

On my system, using fgets, which you used, is about the same speed as using getline. Using fscanf is about 3x slower than these solutions. The fastest method I tested was to use fgetln (available on a Mac from the BSD C library), which was 10-15% faster than fgets/getline.

char *line = NULL;
size_t len = 0;

while ((line = fgetln(f, &len)) != NULL) {
    if (line[len - 1] == '\n') {
        line[len - 1] = '\0';
    }
    //printf("\"%s\"\n", line);
}
if (ferror(f)) {
    perror("fgetln");
}

Another alternative is to use mmap to map the file (only real files though, not pipes, etc) into memory and use string handling directly (strchr etc) to look for line endings. This might be the fastest of all but I didn't test it here.

Generally it is best not to get too hung up about speed unless you know that a piece of code is critical (and normally one doesn't know this). So think more about the advantages and disadvantages of the various solutions:

  • fgets is quick but uses a fixed buffer length and you have to handle overflow as a special case.

  • getline and fgetln are quick, elegant and capture complete lines. These are attractive if we do not need to worry about attacks by huge strings. fgetln is not in any standard so is rather less portable than getline (which seems to be POSIX). I tried fgetln with a file containing one line > INT_MAX chars long (INT_MAX is the limit that fgetln handles) and although it did slow down the system considerably, it failed cleanly with a memory allocation failure:

    fgetln: Cannot allocate memory
    

    getline has a limit of SSIZE_MAX which is much bigger.

  • fscanf - I can't think of a reason to use this for the simple example you pose, but it has its uses if you want to control the input format.

  • mmap is (probably) quick and elegant but only handles real files, which restricts the application a bit (eg you can't just pipe input into the application if it wants to map a file).

\$\endgroup\$
  • \$\begingroup\$ Oops, you are right. My man page says it is in the Standard C Library (libc, -lc), but I guess that does not mean it is in the C standard as the history section says only "The fgetln() function first appeared in 4.4BSD.". I'll edit the answer... \$\endgroup\$ – William Morris Oct 24 '14 at 19:13
3
\$\begingroup\$

It seems silly to have to call strlen() or strtok() to find the end-of-line delimiter, when surely the system could have already known how many bytes were successfully read into buffer.

Also consider how you want to handle the case where the input line is too long for the buffer.

To take care of both issues, I suggest using getline() instead:

char *line = NULL;
size_t buflen = 0;
ssize_t len;

while ((len = getline(&line, &buflen, stdin)) > 0) {
    if (line[len - 1] == '\n') {
        line[--len] = '\0';
    }

    // use buffer
    printf("\"%s\"\n", line);
}
\$\endgroup\$
  • 1
    \$\begingroup\$ I look forward to using getline() more with its allocated buffer once it becomes part of the C standard. Of course one could roll their own getline() in the mean time. A niche area concern that I have with getline() is the inability to limit the line length. A limit is useful to prevent hostile input from creating some huge input aimed to overwhelm code resources. \$\endgroup\$ – chux - Reinstate Monica Oct 23 '14 at 3:15
  • \$\begingroup\$ How would you want the program to behave in the face of unreasonably long input lines? Your two original methods avoid exhaustion, but they don't feel quite correct either. \$\endgroup\$ – 200_success Oct 23 '14 at 3:16
  • \$\begingroup\$ Various approaches: 1) consume input up to a point and toss the rest, then report the error. (prevents OOM) 2) consume and process input up to a limit, recognize it as an attack and report the error (prevent OOM and spinning the CPU) \$\endgroup\$ – chux - Reinstate Monica Oct 23 '14 at 3:24
3
\$\begingroup\$

Why not use the scanf() functionality do the work for you.

char buffer[100];

int  len;
if (fscanf(stdin, "%99[^\n]%n", buffer, &len) != 1)
{
     // No data (or just a '\n')
     len = 0;
}
fscanf(stdin, "%*[^\n]"); /* If the line is longer than 99 this throws away
                           * The rest of the line upto the new line character
                           */
buffer[len] = '\0';
fscanf(stdin, "%*c");     /* read and throw away the newline 
                           */
\$\endgroup\$
  • \$\begingroup\$ Interesting idea this different approach. Forming the needed width of "99" is not much an issue when the buffer size is well know (as in my example), but forming the width when the size is not a literal constant need more code. Doable, but cumbersome. \$\endgroup\$ – chux - Reinstate Monica Oct 23 '14 at 3:04
  • \$\begingroup\$ @chux: Not really that cumbersome if you wrap it in a function. \$\endgroup\$ – Martin York Oct 23 '14 at 4:35
  • \$\begingroup\$ Could use int len = 0; fscanf(stdin, "%99[^\n]%n", buffer, &len); and skip the if() test for same functionality. \$\endgroup\$ – chux - Reinstate Monica Jul 12 '17 at 13:14

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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