Bare-Bones implementation of printf

Question

I made a bare-bones implementation of printf based on an exercise in Chapter 7, problem 5 of "Pointers on C" by Kenneth A. Reek. What criticism do you have of my writing style?

For instance, do you find I added far too many header files in code. Should I add #ifndef guards for the header files? Do you find any of the functions throughout the implementation too difficult too read?

For instance, the myprintf function is the most important function, as it takes advantage of all other functions throughout the program. Is the way myprintf is implemented with switch statements too messy?

#include <stdio.h>
#include <stdarg.h>
#include <stdlib.h>
#include <string.h>
#include <ctype.h>
#include <math.h>
#include <limits.h>
#include <float.h>
#include <assert.h>
#define MAX_LENGTH 1000
void reverse (char *s) {
 char temp; 
 for ( int i = 0, j = strlen(s)-1; i < j; i++, j--)
 {
 temp = s[i];
 s[i] = s[j];
 s[j] = temp;
 }
}
char * printd(long long int n)
{
 static char d[1000];
 static char abs[1000];
 static char * abs_p = abs;
 static int sign;
 if ( n < 0 )
 {
 *d = ('-');
 n = -n;
 }
 while ( lldiv(n,10).quot != 0)
 {
 *abs_p++ = (lldiv(n,10).rem+'0');
 n = (lldiv(n,10).quot);
 }
 reverse(abs);
 strcat(d,abs);
 return d;
}
char * lltoa(long long int n)
{
 static char s[MAX_LENGTH];
 long long int i, sign;
 if ((sign = n) < 0) /* record sign */
 n = -n; /* make n positive by n =-n */
 i = 0;
 do /* generate digits in reverse order */
 {
 s[i++] = lldiv(n,10).rem + '0'; /* get next digit with n % 10 + '0'; */
 } while (n = lldiv(n,10).quot, n > 0); /* delete it with while ( (n /= 10) > 0 )*/
 if (sign < 0)
 s[i++] = '-';
 s[i] = '0円';
 reverse(s);
 return s;
}
char * lltoa2(long long int n)
{
 char d[MAX_LENGTH];
 char abs[MAX_LENGTH];
 char * abs_p = abs;
 int sign;
 if ( n < 0 )
 {
 *d = ('-');
 n = -n;
 }
 while (lldiv(n,10).quot != 0)
 {
 *abs_p++ = (lldiv(n,10).rem+'0');
 n = (lldiv(n,10).quot);
 } 
 *abs_p++ = (lldiv(n,10).rem+'0'); /* first digit after pos or neg sign must be included */
 reverse(abs); /* contents of abs are originally in reverse order of input */
 d[0] = '0円';
 strcat(d,abs);
 return &d[0];
}
double nround(double input, double power)
{
 double marge = pow(10,power);
 double up = input * marge;
 double result = ((double)(llround(up)))/marge;
 return result;
}
// ftoa rounds accurately up to 15 digits, guranteed
// ftoa can only deal with integral values with an absolute value less than or equal to LLONG_MAX
/* ftoa only works until 9.9999999999999999e15 rounded to
zero decimal places
*/
char * ftoa(const double input, const double power) // pow is the '*' in "%.*f"
{
 const double in = nround(input,power); 
 static char a[MAX_LENGTH] = "0円";
 int j = 0;
 while (j < strlen(a) ) a[j++] = '0円';
 long long int f_to_i = (long long int)(in); 
 strcat(a,lltoa(f_to_i)); strcat(a,".");
 double integral = 0;
 double fraction = 0;
 (in >= 0) ? (fraction = modf(in,&integral)): (fraction = modf(-in,&integral)); /* stores fractional part of input */
 char non_zero_mantissa[1000] = "0円"; /* stores non-zero digits in mantissa after leading zeros following decimal point */
 strcat(non_zero_mantissa,lltoa(llround(fraction*pow(10,power))));
 int i = strlen(non_zero_mantissa);
 while ( i++ < power )
 {
 strcat(a,"0");
 }
 strcat(a,non_zero_mantissa);
 return a;
#if 0
 int c = 0;
 while ( a[c] != '0円') putchar(a[c++]);
#endif
}
//#if 0
int myprintf(char const * s,...)
{
 va_list var_arg;
 char * s_p = (char *)(s-1);
 va_start(var_arg,s);
 int ROUND_TO = 0;
 char rounding[1000];
 while (*++s_p != '0円')
 {
 switch(*s_p)
 {
 case '%':
 {
 switch(*++s_p)
 {
 case 'f':
 {
 char const * f_p = ftoa(va_arg(var_arg,double),6);
 while ( *f_p != '0円') putchar(*f_p++);
 break;
 }
 case 'd':
 {
 char const * d_p = (char *)lltoa(va_arg(var_arg,long long int));
 while ( *d_p != '0円') putchar(*d_p++);
 break;
 }
 case 's':
 {
 char const * string_p = va_arg(var_arg,char *);
 while ( *string_p != '0円') putchar(*string_p++);
 break;
 }
 case 'c':
 {
 putchar((char)va_arg(var_arg,int));
 break;
 }
 case '.':
 {
 if (isdigit(*(s_p+1))) //peek after '.' and see if there is a digit
 {
 ROUND_TO = 1;
 int i = 0;
 char * r_p = rounding;
 while (isdigit(*++s_p)) { *r_p++ = *s_p; }
 *r_p = '0円';
 if ( *s_p == 'f' ) 
 {
 myprintf("%s",ftoa(va_arg(var_arg,double),atoi(rounding)));
 ++s_p;
 }
 else
 {
 putchar('%');
 putchar(*s_p);
 myprintf("%s",atoi(rounding));
 }
 }
 else
 {
 putchar('%');
 putchar(*s_p++);
 }
 }
 default:
 {
 putchar(*s_p);
 break;
 }
 }
 break;
 }
 default:
 {
 putchar(*s_p);
 break;
 }
 }
}
 va_end(var_arg);
 return 1;
}
//#endif
//#if 0
int main(void) 
{
#if 0
 ftoa(-3.9999,3);
 putchar('\n');
 ftoa(-1.5555,2);
 putchar('\n');
 ftoa(-3.39823929,5);
 putchar('\n');
 ftoa(-3.0000000000000099,15);
 putchar('\n');
 ftoa(-3.6666666666666666,15);
 putchar('\n');
 ftoa(-3.4545,3);
 putchar('\n');
 ftoa(-3.454599,5);
 putchar('\n');
 ftoa(-1.0000000000000009,15);
 putchar('\n');
 ftoa(3.9999,3);
 putchar('\n');
 ftoa(1.5555,2);
 putchar('\n');
 ftoa(3.39823929,5);
 putchar('\n');
 ftoa(3.0000000000000099,15);
 putchar('\n');
 ftoa(3.6666666666666666,15);
 putchar('\n');
 ftoa(3.4545,3);
 putchar('\n');
 ftoa(3.454599,5);
 putchar('\n');
 ftoa(1.0000000000000009,15);
 putchar('\n');
 ftoa(9.9999999999999999e15,14); //FAILS!!!
 putchar('\n');
myprintf("Thatcher Swag\n%s\n%f\n%c\n%d\n","Swiss Cheese",3.45,'T',3535232523);
double a = 3.14, b = 325.3235, c = 790.866;
myprintf("My bank account amount: %f\n",c);
double light_speed = 3.0e8; 
double planck_mass = 2.17647051*pow(10,-8); 
myprintf("Speed of light: %f\n",light_speed*planck_mass);
printf("Speed of light: %f\n",light_speed*planck_mass); //TESTS PASS
myprintf("%f\n",planck_mass);
printf("%f\n",planck_mass); //TESTS_PASS
#endif
myprintf("%.5f\n",43.235278);
myprintf("%.5f\n",3.14159265);
myprintf("%.3f\n",4.9999);
myprintf("%.10f\n",1558.2392038592972022352698);
myprintf("%.15f\n",1.2392038592972022352698); //myprintf can only round a floating-point that has 15 digits, including before mantissa or less
myprintf("%f\n",323.3435);
myprintf("%s %c %.3f %d\n","Swiss",'A',123.3257,1325352);
myprintf("%s\n",lltoa2(32235235));
myprintf("%s\n",lltoa2(232362351));
myprintf("%s\n",lltoa2(LLONG_MAX));
myprintf("My parents gave me %d hot dogs for July 4\n",359);
}
//#endif

• esote: A good starting point is to look at the actual implementation of GNU C library functions (download the archive file). In this case, look under stdio-common/ and you'll see that printf() is a wrapper around vprintf(), which goes to vfprintf(). If you open stdio-common/vfprintf.c, you'll see their implementation details.
• Phrancis: Welcome to Code Review! I hope you get some great answers.

Answer 1

There is so much going on here that needs improvement that the correctness of converting double to a string has not been assessed yet. I am doubtful that its correctly rounds edge cases. Example

myprintf("%f\n",0.00000049999999999999993); --> 0.000001
// expect 0.000000

---

... I added far too many header files in code(?)

This is a minor concern. Adding some extra standard C header files is rarely a significant mistake.

Should I add #ifndef guards for the header files?

Code should have 3 portions: myprintf.c for int myprintf(char const * s,...), myprintf.h for its declaration and other important constants, defines, non-static functions and a main.c/test.c to show how to use and test myprintf().

myprintf.h should use guards.

Do you find any of the functions throughout the implementation too difficult too read?

Yes.

Opinion: too much vertical white-space.

Strange use of magic numbers. Why 1000 when MAX_LENGTH or another mcaro-constant could be used. Avoid naked magic numbers like 1000

#define MAX_LENGTH 1000
....
static char d[1000];

Mixed use of static. In both cases, better code would pass in the buffer and eschew static buffers.

static char s[MAX_LENGTH]; // in lltoa()
char d[MAX_LENGTH]; // in lltoa2()

Re-use of very common functions name for a non-function with char abs[MAX_LENGTH];. abs() is a very common standard function and use here as a non-function is very distracting.

Unexplained #if 0 blocks of code. If not needed, then delete them before review. Similar issue of //#if 0.

Inconsistent indent depth increment. Sometimes it is 2, sometimes 4. Hmmmm.

Inconsistent vertical white-space. Tip: use an auto formatter. Life is too short to do this/maintain this by hand.

Is the way myprintf is implemented with switch statements too messy?

case '.': is certainly messy.

---

Errors

Failure with various values

myprintf("%d\n",LLONG_MIN); --> "-(\n".
myprintf("%f\n",DBL_MAX); --> "-9223372036854.775391\n"
myprintf("%f\n",-0.0); "0.000000\n", expected "-0.000000\n".
Use signbit() rather than if (sign < 0).

Invalid test

myprintf("%s %c %.3f %d\n","Swiss",'A',123.3257,1325352); passes 1325352 which is an int or long, yet long long expected. Result: UB.

Strange use of llround()

Unclear why code uses limits computation

// double result = ((double)(llround(up)))/marge;
double result = (round(up))/marge;

Failure with 0

lltoa2(0) return an empty string. Use a do{} while loop instead.

Code lacks a test driver

All the myprintf() oblige a in person review of output rather than an automatic one. Test code should compare against the regular printf(). Idea: at the top of myprintf(), add code with vprintf(), then a personal review only need to sight compare pairs of output to see if they math.

Better: re-write code to mysprintf() and then compare string results with sprintf().

The key point is that to write a good myprintf(), you need a mini-tester jig (harness) so automated testing of millions of combination are possible.

Easy stuff

Enable all compiler warnings

Here are some of what came up for me - without even reading the code.
Save time, enable all warnings.

Why sign?

// warning: unused variable 'sign' [-Wunused-variable]
static int sign;

Undefined behavior to return address of local object. This is the biggest error. Until I corrected this, code would not run - and it was so easy to find.

// warning: function returns address of local variable [-Wreturn-local-addr]
char d[MAX_LENGTH];
return &d[0];

Why ROUND_TO?

// warning: variable 'ROUND_TO' set but not used [-Wunused-but-set-variable]
int ROUND_TO = 0;

Small stuff

Undefined behavior

n = -n; is undefined behavior when n == LLONG_MAX in lltoa2().

Replicated use of lldiv()

Instead of 2 calls lldiv(), use one. Or better just code n/10 and n%10. Good compilers will see that and emit one piece of code to calculate both.

reverse(s) unnecessary

Just build string form the end toward the front.

Support functions should be static

Once code is segmented into myprintf() and the rest, functions like lltoa2() should be static to minimize names space pollution.

Odd code

d[0] = '0円'; strcat(d,abs); is same as simpler strcpy(d,abs);

Why "0円" vs ""?

// static char a[MAX_LENGTH] = "0円";
static char a[MAX_LENGTH] = "";

Use of int vs. size_t

size_t is neither too narrow nor too wide for array indexing and sizing. int , in general, may be insufficient. Code like int j = 0; while (j < strlen(a) ) a[j++] = '0円'; raises distracting warnings about mixing int/unsigned math. Instead:

size_t j = 0; while (j < strlen(a) ) {
 a[j++] = '0円';
}

Avoid calling strlen() in a loop.

A weak compiler will call strlen(a) over and over. Each call costs the length of the string.

// int j = 0;
//while (j < strlen(a) ) a[j++] = '0円';
// instead
size_t j = 0;
while (a[j]) {
 a[j++] = '0円';
}
// or simply
memset(a, 0, strlen(a));

On 2nd thought: while (j < strlen(a) ) a[j++] = '0円'; does nothing useful after static char a[MAX_LENGTH] = "0円"; as strlen(a) is 0.

Review printf() signature

printf() uses restrict. This may be advanced for OP at this time, but worthy of research.

int myprintf(char const * s,...)
int printf(const char * restrict format, ...);

---

Recommendation:

You are attempting one of the most challenging function re-writes - a bold task. I commend you on your bravery.

Try again with just integer types. After completing that, move on to floating-point.

• Lundin: Very good review. I only have one thing to add, and that is to avoid the atoi family of functions since they don't have proper error handling upon encountering unexpected input, which will be extra important here. Should be swapped out for strtol family of functions.
• chux: @Lundin Agreed. There are many places assuming benign input. Robust code would instead gracefully handle extreme/hostile input.
• AminM: @chux thank you for review , can you put complete code here?
• chux: @AminM Your question is unclear. Are you asking for me to post a complete well functioning code for myprintf()?

Answer 2

Some small items, additional to the great review by Chux.

• You don't seem to use anything from <assert.h> or <float.h>, so no need to include those.
• The missing features are not documented anywhere, so we have to guess what subset of printf() is supported.
• Test cases are missing for ±∞ and not-a-number values with the %f specifier.

• Precision specifiers don't work properly with %s:

  myprintf("%.5s", "abcdefg"); // crashes
  

• Unnecessary obfuscation: *(s_p+1) is much more recognisable when written as s_p[1].

• The fallthrough from case '.' to default causes the second conversion specifier to be printed literally in this example (and could be even more serious if the second conversion were %%):

  myprintf("%.1f%f\n", 1.0, 0.0);
  

• chux: "Test cases ... null pointers with the %s specifier": Hmmm. As printf("%s", (void*) NULL) is UB, testing myprintf() would seem optional, though perhaps informative, as any result would match UB.
• Toby Speight: Yes, that is UB, so doesn't need a test case. I confess I was thinking ahead to an implementation of %p - it's useful to have a distinct representation for null pointers there. I'll remove that recommendation.