Sample printf implementation

Question

Here is my code for printf() implementation in C. Please share your thoughts for improvements.

#define PRINTCHAR(c) putchar(c)
#define MAXLEN 256

// Write a printf function which simulates printf() in C.
int formatAndPrintString(char *pcFmt, va_list lList)
{
    //Holds the number of characters printed, 0 on error
    int nChars = 0;

while(*pcFmt != '\0')
{
    if(*pcFmt == '%')
    {
        pcFmt++;

        int iTemp = 0;
        char acTemp[MAXLEN] = ""; 
        char *pcTemp = NULL, cTemp = 0;

        switch (*pcFmt)
        {
        case 'd':
            iTemp = va_arg(lList, int);
            itoa(iTemp, acTemp, 10); 

            for(int i = 0; i < strlen(acTemp); i++)
            {
                PRINTCHAR(acTemp[i]); nChars++;
            }
            break;
        case 's':
            pcTemp = va_arg(lList, char*);

            while(*pcTemp != '\0')
            {
                PRINTCHAR(*pcTemp); nChars++;
                pcTemp++;
            }
            break;
        case 'c':
            cTemp = va_arg(lList, int);  
            PRINTCHAR(cTemp);
            nChars++;
            break;
       default:
          break;
       }
       pcFmt++;
    }
    else 
    // print the char to console
    {
       PRINTCHAR(*pcFmt);
       pcFmt++; nChars++;
    }
}
return nChars;
}

// Print function identical to printf() in stdio.h
int printfunc(char *pcFormat, ...)
{
    if(!pcFormat)
    {
        return 0;
    }

    va_list lList;
    va_start(lList, pcFormat);

    return(formatAndPrintString(pcFormat, lList));
}

Answer 1

Bugs

There is no call to va_end()

 The va_start() macro must be called first, and it initializes ap, which can be passed to va_arg() for each argument to be processed.  Calling
 va_end() signals that there are no further arguments, and causes ap to be invalidated.  Note **that each call to va_start() must be matched by a
 call to va_end(), from within the same function**.

Char and Int do not have the same size.

This advice was wrong:

    case 'c':
        cTemp = va_arg(lList, int);  

I am surprised this passes your unit tests. It will read a value off the parameter list but also advance it sizeof(int) places. While a character is only of length sizeof(char).

Note  sizeof(int) > sizeof(char)
      sizeof(char) == 1

I am going to leave the above in (because it is a learning experience for me).

From: @PeteBecker C99 Section: 6.5.2.2/7

If the expression that denotes the called function has a type that does include a prototype, the arguments are implicitly converted, as if by assignment, to the types of the corresponding parameters, taking the type of each parameter to be the unqualified version of its declared type. The ellipsis notation in a function prototype declarator causes argument type conversion to stop after the last declared parameter. The default argument promotions are performed on trailing arguments.

Thus the char argument is promoted to int when effectively pushed to the stack. So you need to use int when reading it from the var args list. Thus the code is correct.

Unchecked increment:

 default:
          break;
       }
       pcFmt++;

Pretty sure this would break if I used the string "%"

Notes:

Whenever you call PRINTCHAR() you also execute nChars++; so why not put both into the same functioin.

Extensions you should add in the long run.

You cover the bare minimum

You have format d/s/c but in printf there are a lot more with a lot more options: nore Types, type Extension (long/short), width, more formatting, etc.

No Locale Coverage

Output printing is affected a lot by current locale.

Answer 2

Going from case 'd' to case 'c', you have these lines:

'd':

PRINTCHAR(acTemp[i]); nChars++;

's':

PRINTCHAR(*pcTemp); nChars++;

'c':

PRINTCHAR(cTemp);
nChars++;

Why did you all of a sudden change the pattern? In my opinion, you should be using the pattern that you used in option 'c' because it is clean and spread out, rather than jammed into one line.

---

Your while loop looks a lot like a for loop:

while(*pcFmt != '\0')
{
    ...
        pcFmt++;

I recommend changing it to a for loop so all the components of the loop are easily seen together in the signature of the loop:

for(; *pcFmt != '\0'; pcFmt++)

Also, from the code that I've seen that iterates through the characters of a string, this way (as I have shown) is more common.

---

Your indentation is really off.

That while loop and everything inside of should be indented because it is inside a function. And, the case statements inside your switch should, too, be indented.

---

I'm not sure if you gave yourself a requirement to not use any string printing functions, but these lines:

while(*pcTemp != '\0')
{
    PRINTCHAR(*pcTemp); nChars++;
    pcTemp++;
}

could be shortened to

puts(pcTemp);

The puts function is a built in function that just comes with the C compiler; no <stdio.h> needed.

---

Answer 3

Termination expression

I have a pet peeve when people use functions/expressions in their termination clause which have constant value:

        for(int i = 0; i < strlen(acTemp); i++)

Here you are going to call strlen N times, making this loop an \$O(n^2)\$ loop for no reason. Much better would be:

        int len = strlen(acTemp);
        for(int i = 0; i < len; i++)

I just did a check and gcc -O3 is smart enough to optimize that loop for you. But there will come a time where it won't be able to do that because instead of strlen you will have a function that the compiler doesn't recognize, and it will be forced to call that function N times.