7
\$\begingroup\$

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));
}
\$\endgroup\$
10
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ agreed it will break in the default case, and I implemented 3 basic formats(s,d,c) to begin with,your feedback again much appreciated,tried to implement with straight and clean approach \$\endgroup\$ – Subhajit Jul 9 '15 at 17:20
  • 2
    \$\begingroup\$ If char is smaller than int (that's not required: char can be large) then char arguments are promoted to int when passed through an ellipsis. So the use of va_arg(lList, int) for 'c', while unintuitive, is correct. \$\endgroup\$ – Pete Becker Jul 9 '15 at 23:54
  • \$\begingroup\$ @PeteBecker: You have a reference for that? \$\endgroup\$ – Martin York Jul 10 '15 at 0:01
  • \$\begingroup\$ Read about "default promotions". \$\endgroup\$ – Pete Becker Jul 10 '15 at 0:06
  • \$\begingroup\$ @PeteBecker: I am aware of the default promotions. But why is the compiler promoting types when it knows the types. Do you have something about va_arg() or about placing things on the stack when elipses are involved that shows that they will be promoted. \$\endgroup\$ – Martin York Jul 10 '15 at 0:10
8
\$\begingroup\$

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.


\$\endgroup\$
  • \$\begingroup\$ Thanks @SirPython, for in case of while loop and puts() call against putchar() is much appreciated,will use that,tried to maintain a straightforward approach to the problem \$\endgroup\$ – Subhajit Jul 9 '15 at 17:24
7
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ I think GCC and Clang are always able to fix the mistake of calling strlen repeatedly with the same input because in their implementation the function is marked with __attribute__((pure)), so redundant calls can be eliminated in the common subexpression removal step. Nevertheless, your answer is valid. The OP could very well be calling some user defined function inside the loop instead, that the compiler could not be able to optimize. \$\endgroup\$ – glampert Jul 10 '15 at 2:35

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.