HiveBrain v1.2.0
Get Started
← Back to all entries
patterncMinor

My own snprintf implementation in C

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
implementationownsnprintf

Problem

I decided to make my own version of snprintf in C. I intentionally changed some things though. My version guarantees the buffer printed to will be null-terminated, and it returns the number of characters printed to the buffer, not the number that would have been printed if the buffer's size was not limited. And I only worried about some of the main formatting features, like %s, %c, %d, %h, and %H.

I would love to know what I could do better in this and improve, or what aspects of it I did or did not implement well.

```
#include
#include
#include
#include
#include

int INT_TO_STR_DIGITS_L[16] = { '0', '1', '2', '3', '4', '5', '6', '7', '8', '9', 'a', 'b', 'c', 'd', 'e', 'f' };
int INT_TO_STR_DIGITS_U[16] = { '0', '1', '2', '3', '4', '5', '6', '7', '8', '9', 'A', 'B', 'C', 'D', 'E', 'F' };

int int_to_str(int x, char *buf, size_t size, int base, int uppercase) {
int length = (int)ceil(log((double)x)/log((double)base));
int r, i = 0;
char c;

if (size = size) break;
r = x % base;
if (uppercase) {
c = INT_TO_STR_DIGITS_U[r];
} else {
c = INT_TO_STR_DIGITS_L[r];
}
buf[length-i-1] = c;
x /= base;
i++;
} while (x != 0);

return i;
}

int my_snprintf(char str, size_t max_size, const char fmt, ...) {

va_list arg_list;
va_start(arg_list, fmt);

int chars_printed = 0;
char *start_str = str;

char c, *str_arg;
int num, len;
int uppercase = 0, base = 10;

for (int i = 0; fmt[i] != 0; i++) {
if (max_size - chars_printed <= 0) {
break;
} else if (fmt[i] == '%') {
i++;
switch (fmt[i]) {
case 'c':
c = va_arg(arg_list, int);
str[chars_printed++] = c;
break;
case '%':
str[chars_printed++] = '%';
break;
case 's':
str_arg = va_arg(arg_list, char *);

Solution

Bug

Your int to string conversion isn't working correctly when the number being printed is an exact power of the base. Here is a program that demonstrates the bug:

int main(void)
{
    char buf[256];

    memset(buf, 'z', 256);
    my_snprintf(buf, 256, "abc%ddef", 1000);
    printf("%s\n", buf);
}


Expected output:

abc1000def


Actual output:

ab1000zdef


As you can see, the 1000 portion was written one too far to the left. The problem is that your number length computation is off by one for exact powers of the base.
Unnecessary and unsafe floating point operations

The Floating Point Police™ would like to point out that the use of floating point in int_to_str() is both unnecessary and dangerous. First of all, this line:

int length = (int)ceil(log((double)x)/log((double)base));


could be rewritten to use a loop to count the number of digits. By using floating point, you open yourself up to rounding errors. For example, if x were 125 and base were 5, you would expect length to be 3. However, when I ran the above code using 125 and 5 on my x86 machine, I got a length of 4 instead. This is because the division evaluated to something like 3.00000001 and ceil rounded it up to 4. (Of course there is already an unrelated off by one bug mentioned in the previous section. This floating point use is a separate cause of concern).

The same thing applies to this line:

x /= (int)pow(base, (float)(length - size));


This could be rewritten to be a loop where you divide by base once per loop iteration. By using pow() and casting to int, you run the risk of the result of pow erroneously rounding down to the previous int.

Code Snippets

int main(void)
{
    char buf[256];

    memset(buf, 'z', 256);
    my_snprintf(buf, 256, "abc%ddef", 1000);
    printf("%s\n", buf);
}
int length = (int)ceil(log((double)x)/log((double)base));
x /= (int)pow(base, (float)(length - size));

Context

StackExchange Code Review Q#132860, answer score: 8

Revisions (0)

No revisions yet.