7
\$\begingroup\$

I am trying to do a few exercises from the K&R book from the first chapter - exercise 1-19. I have to write a program "detab" which replaces tabs in the input with the proper number of blanks to the next tab stop. Assume a fixed set of tab stops, say every n position.

Here is what I've written so far (My Solution):

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

#define TAB     '\t'
#define SPACE       ' '
#define DEFTABSIZE  4   /* default tab size */

#define calc_n_spaces(x, y) ( (y - (x % y)) )

void detab(const char *restrict);

int
main(void)
{
    char *buf = NULL;
    size_t bufsiz = 0;

    while (getline(&buf, &bufsiz, stdin) != -1)
        (void) detab(buf);

    free(buf);
    exit(EXIT_SUCCESS);
}


void
detab(const char *restrict from)
{
    int i, j, spaces;
    char to[BUFSIZ];

    i = j = 0;
    while ((to[i] = from[i]) != '\0') {
        if (to[i] == TAB) {
            spaces = calc_n_spaces(j, DEFTABSIZE);  /* calculate number of spaces. */
            while (spaces-- > 0) {
                (void) putchar(SPACE);
                ++j;
            }
        } else {
            (void) putchar(to[i]);
            ++j;    
        }
        ++i;
    }
    return;
}

I'd like to know if there is a way I can improve it.

Note: I am using the 1st version of the book.

\$\endgroup\$
2
  • 1
    \$\begingroup\$ Which C standard are you writing for? C89 or C99 or something else? \$\endgroup\$
    – qwr
    Commented Aug 20, 2022 at 21:47
  • \$\begingroup\$ C99. Although I don't know if it would be convenient to stick with what they show in the chapters to solve the exercises. \$\endgroup\$ Commented Aug 20, 2022 at 22:30

3 Answers 3

9
\$\begingroup\$

When defining macros with arguments, it's good practice to avoid precedence-related surprises by enclosing those arguments with parens in the expansion:

#define calc_n_spaces(x, y) ((y) - (x) % (y))

As another answer observes, a simple inlinable function is more appropriate here, and that avoids repeating any side-effects in y. Or, since it's used only once, just inline it yourself.


The restrict qualifier on detab()'s argument doesn't add anything, as there's nothing in the function which could be aliased to the argument. Just omit it.


We always return a success status, but there's plenty that can go wrong in reading or writing to streams. We need to check every I/O operation for errors and respond accordingly.


The detab() function runs into undefined behaviour as soon as we encounter a line with BUFSIZ or more characters. However, we don't really use to anywhere (the two places we read from it, we just get the value we recently assigned, i.e. from[i]), so we can remove it, fixing this bug:

    while (from[i] != '\0') {
        if (from[i] == TAB) {
            int spaces = calc_n_spaces(j, DEFTABSIZE);  /* calculate number of spaces. */
            while (spaces-- > 0) {
                putchar(SPACE);
                ++j;
            }
        } else {
            putchar(from[i]);
            ++j;
        }
        ++i;
    }

The variable names i and j aren't very informative here. From reading the code, it seems that i is the logical (input) column number, and j is the output(physical) column number.


There's no need to cast the return value of functions that are ignored. It's particularly odd to read that in main() given that detab() is declared as returning void anyway.


The getline() function is not standard C (it's required by POSIX, but not by the C standard). However, we don't need to read a whole line before working on it, as the algorithm only considers a character at a time. We just need to reset our column number when we see a newline.

Here's a vastly modified version that does exactly that:

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

#define TAB_STOP  8

int main(void)
{
    int col = 0;                /* logical column number */
    for (;;) {
        int c = getchar();

        switch (c) {
        case EOF:
            return ferror(stdin) ? EXIT_FAILURE : EXIT_SUCCESS;
        case '\t':
            {
                int spaces = TAB_STOP - col % TAB_STOP;
                if (printf("%.*s", spaces, "        ") != spaces) {
                    return EXIT_FAILURE;
                }
                col += spaces;
                break;
            }
        case '\r':
        case '\n':
            col = -1;
            /* FALLTHROUGH */
        default:
            if (putchar(c) == EOF) {
                return EXIT_FAILURE;
            }
            ++col;
        }
    }
}

Note the checking of more error conditions, too.

\$\endgroup\$
9
  • \$\begingroup\$ Hey, @Toby Speight. I appreciate the time you took to write all that in detail. They really are very helpful, I will consider them from now on. \$\endgroup\$ Commented Aug 20, 2022 at 22:54
  • \$\begingroup\$ "There's no need to cast the return value of functions that are ignored. " Various warnings whine when a return value is not used. (void) cast quiets that. C2x likely to have some style attribute on return value to address that. \$\endgroup\$ Commented Aug 20, 2022 at 23:32
  • 2
    \$\begingroup\$ return feof(stdin) ? EXIT_SUCCESS : EXIT_FAILURE; has a slight advantage over return ferror(stdin) ? EXIT_FAILURE : EXIT_SUCCESS;. \$\endgroup\$ Commented Aug 20, 2022 at 23:37
  • 3
    \$\begingroup\$ printf("%.*s", spaces, " ") has trouble with TAB_STOP > 8. Maybe printf("%*s", spaces, "")? \$\endgroup\$ Commented Aug 20, 2022 at 23:43
  • 1
    \$\begingroup\$ Well, it would be an odd implementation that marked strcpy() as noignore. Existing implementations use their (not yet standard) attributes for functions where ignoring the return value is likely a bug, such as scanf(). And the functions in this program do return useful values that should be checked (apart from detab() itself, which returns no value, so certainly shouldn't be cast). \$\endgroup\$ Commented Aug 21, 2022 at 11:37
8
\$\begingroup\$

General Observations

There are problems using the first version of the book: the C programming language changed a great deal when it switched to ANSI in 1989. Prefer the second version if you can get it; there are also more modern books written by other authors.

Some of the differences are how parameters are declared in functions. Then there is the fact that the ANSI standard allowed variables to be declared anywhere while the original requires variable declarations to be at the top of their containing block.

Calling exit(status) From Main

The default behavior of int main() is to return the status of the program to the operating system; there is no need to ever call exit() from main. The exit() function is there so that the C program can exit from other functions besides main in case of error situations.

Prefer Functions Over Macros

The macro calc_n_spaces() would be better as either a function or inline code. The current implementation doesn't save either programming space (lines of code) or performance. Modern optimizing compilers might very turn a function into inline code.

Code Organization

Function prototypes are very useful in large programs that contain multiple source files, and that in case they will be in header files. In a single file program like this it is better to put the main() function at the bottom of the file and all the functions that get used in the proper order above main(). Keep in mind that every line of code written is another line of code where a bug can crawl into the code.

\$\endgroup\$
2
  • 3
    \$\begingroup\$ Actually, the first ANSI/ISO Standard C required declarations to precede statements within each block. It wasn't until (IIRC) C99 that the more relaxed ordering rules were introduced. It looks like K&R 2 is being used here, given the function prototypes. \$\endgroup\$ Commented Aug 20, 2022 at 13:51
  • \$\begingroup\$ @pacmaninbw Thanks, I'll take your comments into consideration, and I'll try to see if I can find the 2nd version or a modern version. \$\endgroup\$ Commented Aug 20, 2022 at 22:40
4
\$\begingroup\$

Although I don't know much about C, I have to say that I could easily read the code, and understand everything.

However...

Space output

Why not generate the string with spaces, and output that instead of looping for every character?

This could probably make your code run faster, but, it seems that C and strings aren't really good friends...

Tab size...

Currently, you have this:

#define DEFTABSIZE  4   /* default tab size */

This assumes that a tab is 4 spaces, which isn't always the case.

A few examples:

Window's Command Line - 8 characters:

Tab on CMD

Notepad - 8 characters:

Tab on Notepad

HTML - 8 characters:

<pre>This is how wide a tab is:
&#9;&lt;</pre>

VSCode - 4 characters:

Tab on VSCode

However, for YALM and Compose, the size is just 2.

Notepad++ - 4 characters:

Tab on Notepad++

This value can be edited.

StackSnippet - 2 characters:

Tab on StackSnippet


In short, you can't assume the size of a tab character.
Your choice of tab size is made a little more odd considering that the tab occupies 8 characters in the terminal.
Specially since you have a terminal application.


The C language already gives you a way to read command line arguments.
You can just use int main(int argc, char **argv) instead of int main(void), according to this: https://stackoverflow.com/questions/5157337/reading-command-line-parameters

You can, for example, check the 2nd argument to see if it is a number.
For example:

#include <stdlib.h>

[...]
int main(int argc, char **argv)
{
    // long because of strtol()
    long tabsize = DEFTABSIZE;
    
    // argv[0] has the executable path
    if(argc > 1)
    {
        char *ptr;
        long number = strtol(argv[1], &ptr, 10);
        
        // 1 to 10 spaces, for example.
        // if(number && ...) would cause issues with
        //     negative numbers, as they are lower than 10.
        if(number > 0 && number <= 10)
        {
            tabsize = number;
        }
    }

    [...]
    
    while (getline(&buf, &bufsiz, stdin) != -1)
        (void) detab(buf, tabsize);

    [...]
}

Or adapt one of the existing answers to support this.

Strings and chars

Your code, and all answers, take absolutely no consideration at all for strings and chars.

This is a big issue, as, for example, if you're writing YALM or doing something where whitespaces matter (like console output), you can ruin it.

Usually, this shouldn't be that big of an issue, but when it is an issue, it is a BIG issue.

Here's a very simple situation where it will lead to issues:

char TAB = '    ';

(I'm aware you can just use the \t escape sequence, but this is just an example. Also, the rendered output swaps the actual tab for 4 spaces.)

If you use Toby Speight's answer, you can easily keep track if you're inside a string/char or not, and act accordingly.

\$\endgroup\$
9
  • 3
    \$\begingroup\$ Any not-completely-rubbish editor will allow you to set pretty much any tab and indentation size, (but they do of course have defaults). \$\endgroup\$
    – Oskar Skog
    Commented Aug 21, 2022 at 10:16
  • 3
    \$\begingroup\$ Terminals also support setting tab stops at arbitrary columns. "\e[0g" clears tab stop at current position, "\e[3g" clears all, "\eH" sets tab stop at current position. \$\endgroup\$
    – Oskar Skog
    Commented Aug 21, 2022 at 10:25
  • 2
    \$\begingroup\$ "Why not generate the string with spaces," --> the destination string could be much longer than the original. This obliges a worse case size like strlen(buf)*DEFTABSIZE + 1, a 2 pass over buf to pre-calculate the size or other code, simply to find the generated string' size. Do-able, yet costs either way. \$\endgroup\$ Commented Aug 21, 2022 at 11:11
  • 3
    \$\begingroup\$ isdigit(argv[1]) is UB as isdigit() is not for pointers, but for unsinged char values and EOF. \$\endgroup\$ Commented Aug 21, 2022 at 11:14
  • 3
    \$\begingroup\$ isdigit(argv[1]) is an invalid call to isdigit(). To test if a string is numeric, like "123" or "-87654321", but not "", "abc", "123xyz", start with strtol(). \$\endgroup\$ Commented Aug 21, 2022 at 20:41

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.