Try to improve the fixedpt_str function and add unit tests for it (#1054)

* Try to improve the fixedpt_str function and add unit tests for it

* Fix overflow in fixedpt_fracpart macro

* Futher clarify the function

* Clarify fixed point string printing test suite name

* Clearer handling of negative numbers

* Apply suggestions from code review

---------

Co-authored-by: Magnus Larsen <[email protected]>

Author: Vagabond

src/game/objects/har.c

@@ -308,7 +308,7 @@ void har_walk_to(object *obj, int destination) {
 
     fixedpt vx = h->fwd_speedf * object_get_direction(obj);
     char vx_buf[FIXEDPT_STR_BUFSIZE];
-    fixedpt_str(vx, vx_buf, -1);
+    fixedpt_str(vx, vx_buf, sizeof(vx_buf), -1);
     log_debug("set velocity to %s", vx_buf);
     object_set_vel(obj, vec2f_createf(vx, 0));
 
@@ -512,7 +512,7 @@ void har_move(object *obj) {
         return;
     } else if(h->walk_destination > 0) {
         char posx_buf[FIXEDPT_STR_BUFSIZE];
-        fixedpt_str(obj->pos.fx, posx_buf, -1);
+        fixedpt_str(obj->pos.fx, posx_buf, sizeof(posx_buf), -1);
         log_debug("still walking to %d, at %s", h->walk_destination, posx_buf);
     }
 
@@ -1235,8 +1235,8 @@ int har_collide_with_har(object *obj_a, object *obj_b, int loop) {
     if(obj_b->gs->match_settings.rehit && b->state == STATE_FALLEN &&
        (!object_is_airborne(obj_b) || b->endurance <= 0)) {
         char xbuf[FIXEDPT_STR_BUFSIZE], ybuf[FIXEDPT_STR_BUFSIZE];
-        fixedpt_str(obj_b->pos.fx, xbuf, -1);
-        fixedpt_str(obj_b->pos.fy, ybuf, -1);
+        fixedpt_str(obj_b->pos.fx, xbuf, sizeof(xbuf), -1);
+        fixedpt_str(obj_b->pos.fy, ybuf, sizeof(ybuf), -1);
         log_debug("REHIT is not possible %d %s %s %d", object_is_airborne(obj_b), xbuf, ybuf, b->endurance);
         return 0;
     }
@@ -1281,15 +1281,15 @@ int har_collide_with_har(object *obj_a, object *obj_b, int loop) {
                 // TODO use 90% of the block pushback as cornerpush for now
                 fixedpt push = -1 * object_get_direction(obj_a) *
                                ((move->block_stun - 2) * fixedpt_rconst(0.74 * 0.9) + fixedpt_rconst(0.9));
-                fixedpt_str(push, pushbuf, -1);
+                fixedpt_str(push, pushbuf, sizeof(pushbuf), -1);
                 log_debug("doing block cornerpush of %s", pushbuf);
                 vel.fx += push;
                 object_set_vel(obj_a, vel);
             } else {
                 vec2f vel = object_get_vel(obj_b);
                 fixedpt push = -1 * object_get_direction(obj_b) *
                                ((move->block_stun - 2) * fixedpt_rconst(0.74) + fixedpt_rconst(1));
-                fixedpt_str(push, pushbuf, -1);
+                fixedpt_str(push, pushbuf, sizeof(pushbuf), -1);
                 log_debug("doing block pushback of %s", pushbuf);
                 vel.fx += push;
                 object_set_vel(obj_b, vel);
@@ -1451,8 +1451,8 @@ void har_collide_with_projectile(object *o_har, object *o_pjt) {
     if(o_har->gs->match_settings.rehit && h->state == STATE_FALLEN &&
        (!object_is_airborne(o_har) || h->endurance <= 0)) {
         char xbuf[FIXEDPT_STR_BUFSIZE], ybuf[FIXEDPT_STR_BUFSIZE];
-        fixedpt_str(o_har->pos.fx, xbuf, -1);
-        fixedpt_str(o_har->pos.fy, ybuf, -1);
+        fixedpt_str(o_har->pos.fx, xbuf, sizeof(xbuf), -1);
+        fixedpt_str(o_har->pos.fy, ybuf, sizeof(ybuf), -1);
         log_debug("REHIT is not possible %d %s %s %d", object_is_airborne(o_har), xbuf, ybuf, h->endurance);
         return;
     }

src/game/protos/object.c

@@ -579,8 +579,8 @@ void object_set_animation(object *obj, animation *ani) {
     // Debug texts
     if(obj->cur_animation->id == -1) {
         char bufx[FIXEDPT_STR_BUFSIZE], bufy[FIXEDPT_STR_BUFSIZE];
-        fixedpt_str(obj->pos.fx, bufx, -1);
-        fixedpt_str(obj->pos.fy, bufy, -1);
+        fixedpt_str(obj->pos.fx, bufx, sizeof(bufx), -1);
+        fixedpt_str(obj->pos.fy, bufy, sizeof(bufy), -1);
         log_debug("Custom object set to (x,y) = (%s,%s).", bufx, bufy);
     } else {
         /*log_debug("Animation object %d set to (x,y) = (%f,%f) with \"%s\".", */

src/utils/fixedptc.h

@@ -84,6 +84,7 @@ typedef uint32_t fixedptud;
 #define FIXEDPT_MIN INT16_MIN
 #define FIXEDPT_MAX INT16_MAX
 #define FIXEDPTU_MAX UINT16_MAX
+#define FIXEDPT_WCHARS 5 // 16 bit numbers can use at most 5 decimal digits
 #elif FIXEDPT_BITS == 32
 typedef int32_t fixedpt;
 typedef int64_t fixedptd;
@@ -92,14 +93,16 @@ typedef uint64_t fixedptud;
 #define FIXEDPT_MIN INT32_MIN
 #define FIXEDPT_MAX INT32_MAX
 #define FIXEDPTU_MAX UINT32_MAX
+#define FIXEDPT_WCHARS 10 // 32 bit numbers can use at most 10 decimal digits
 #elif FIXEDPT_BITS == 64
 typedef int64_t fixedpt;
 typedef __int128_t fixedptd;
 typedef uint64_t fixedptu;
 typedef __uint128_t fixedptud;
 #define FIXEDPT_MIN INT64_MIN
 #define FIXEDPT_MAX INT64_MAX
-#define FIXEDPTU_MAX UINT64_MAX
+#define FIXEDPTU_MAX UINT64_MAX // 64 bit numbers can use at most 20 decimal digits
+#define FIXEDPT_WCHARS 20
 #else
 #error "FIXEDPT_BITS must be equal to 16, 32, or 64"
 #endif
@@ -125,7 +128,7 @@ typedef __uint128_t fixedptud;
 #define fixedpt_xmul(A, B) ((fixedpt)(((fixedptd)(A) * (fixedptd)(B)) / FIXEDPT_ONE))
 #define fixedpt_xmul3(A, B, C) fixedpt_xmul(fixedpt_xmul((A), (B)), (C))
 #define fixedpt_xdiv(A, B) ((fixedpt)(((fixedptd)(A) * FIXEDPT_ONE) / (fixedptd)(B)))
-#define fixedpt_fracpart(A) ((fixedpt)(A) & FIXEDPT_FMASK)
+#define fixedpt_fracpart(A) ((fixedptud)(A) & FIXEDPT_FMASK)
 
 #define FIXEDPT_ONE ((fixedpt)((fixedpt)1 << FIXEDPT_FBITS))
 #define FIXEDPT_ONE_HALF (FIXEDPT_ONE >> 1)
@@ -194,66 +197,110 @@ static inline fixedpt fixedpt_div(fixedpt A, fixedpt B) {
  * of decimal digits will be used (2 for 32-bit fixedpt width, 10 for
  * 64-bit fixedpt width); If set to -2, "all" of the digits will
  * be returned, meaning there will be invalid, bogus digits outside the
- * specified precisions.
+ * specified precisions. If set to -3, all the zero digits will be trimmed
+ * from the end of the decimal digits.
+ *
+ * Returns the number of bytes written to `str`, not counting the null terminator.
  */
-static inline void fixedpt_str(fixedpt A, char *str, int max_dec) {
+static inline size_t fixedpt_str(fixedpt A, char *str, size_t bufsize, int max_dec) {
     // boy this function sure is safe with pointers :(
-    int ndec = 0, slen = 0;
-    char tmp[12] = {0};
+    size_t nint = 0;  // number of integer digits
+    size_t nfrac = 0; // number of fractional digits
+    size_t slen = 0;  // string length used
+
+    // accumulator for integer digits, can hold the max number of digits for FIXEDPT_BITS
+    char acc[FIXEDPT_WCHARS] = {0};
     fixedptud fr, ip;
+    fixedptud magnitude = (fixedptud)(fixedptd)A;
     const fixedptud one = (fixedptud)1 << FIXEDPT_BITS;
     const fixedptud mask = one - 1;
+    size_t num_places = 0;
 
-    if(max_dec == -1)
+    // return if the destination is null or too small
+    if(str == NULL || bufsize <= 0) {
+        return 0;
+    } else if(bufsize < 2) {
+        str[0] = '\0';
+        return 0;
+    }
+
+    if(max_dec == -1) {
 #if FIXEDPT_BITS == 16 || FIXEDPT_BITS == 32
 #if FIXEDPT_FBITS < 16
-        max_dec = 2;
+        num_places = 2;
 #else
-        max_dec = 4;
+        num_places = 4;
 #endif
 #elif FIXEDPT_BITS == 64
-        max_dec = 10;
+        num_places = 10;
 #else
 #error Invalid width
 #endif
-    else if(max_dec == -2)
-        max_dec = 15;
+    } else if(max_dec == -2) {
+        num_places = 15;
+    } else if(max_dec == -3) {
+        num_places = 15;
+    } else if(max_dec < 0) {
+        // other negative values of max_dec not allowed
+        return 0;
+    } else {
+        num_places = max_dec;
+    }
 
     if(A < 0) {
         str[slen++] = '-';
-        A *= -1;
+        magnitude *= -1;
     }
 
-    ip = fixedpt_toint(A);
+    // Accumulate the integer digits in reverse order
+    // by dividing the integer component by 10 each time.
+    // Use do/while so at least a 0 is written
+    ip = fixedpt_toint(magnitude);
     do {
-        tmp[ndec++] = '0' + ip % 10;
+        acc[nint++] = '0' + ip % 10;
         ip /= 10;
-    } while(ip != 0);
+    } while(ip != 0 && nint < sizeof(acc) - 1);
+
+    // reverse the accumulated digits into the output string
+    while(nint > 0 && slen < bufsize - 1) {
+        str[slen++] = acc[--nint];
+    }
 
-    while(ndec > 0)
-        str[slen++] = tmp[--ndec];
-    str[slen++] = '.';
+    // check if we want, and have space for, decimal digits
+    if(num_places > 0 && slen < bufsize - 1) {
+        str[slen++] = '.';
 
-    fr = (fixedpt_fracpart(A) << FIXEDPT_WBITS) & mask;
-    do {
-        fr = (fr & mask) * 10;
+        fr = (fixedpt_fracpart(magnitude) << FIXEDPT_WBITS) & mask;
+        while(nfrac < num_places && slen < bufsize - 1) {
+            fr = (fr & mask) * 10;
 
-        str[slen++] = '0' + (fr >> FIXEDPT_BITS) % 10;
-        ndec++;
-    } while(fr != 0 && ndec < max_dec);
+            str[slen++] = '0' + (fr >> FIXEDPT_BITS) % 10;
+            nfrac++;
+        }
 
-    if(ndec > 1 && str[slen - 1] == '0')
-        str[slen - 1] = '\0'; /* cut off trailing 0 */
-    else
-        str[slen] = '\0';
+        // trim any trailing 0s, if desired
+        while(max_dec == -3 && nfrac > 0 && str[slen - 1] == '0') {
+            slen--;
+            nfrac--;
+        }
+
+        // if we didn't add any decimal values, omit the decimal place
+        if(nfrac == 0) {
+            slen--;
+        }
+    }
+
+    size_t bytes_used = slen < bufsize ? slen : bufsize - 1;
+    str[bytes_used] = '\0';
+    return bytes_used;
 }
 
 /* Converts the given fixedpt number into a string, using a static
  * (non-threadsafe) string buffer */
 static inline char *fixedpt_cstr(const fixedpt A, const int max_dec) {
     static char str[25];
 
-    fixedpt_str(A, str, max_dec);
+    fixedpt_str(A, str, sizeof(str), max_dec);
     return (str);
 }
 

testing/test_fixedptc_str.c

@@ -0,0 +1,152 @@
+#include "utils/fixedptc.h"
+#include <CUnit/CUnit.h>
+
+void test_zero_value(void) {
+    char buf[FIXEDPT_STR_BUFSIZE];
+    fixedpt zero = 0;
+    size_t written;
+
+    // Default precision (-1)
+    written = fixedpt_str(zero, buf, sizeof(buf), -1);
+    CU_ASSERT_STRING_EQUAL(buf, "0.00");
+    CU_ASSERT_EQUAL(written, 4);
+
+    // Trim all decimals (-3)
+    written = fixedpt_str(zero, buf, sizeof(buf), -3);
+    CU_ASSERT_STRING_EQUAL(buf, "0");
+    CU_ASSERT_EQUAL(written, 1);
+}
+
+void test_positive_integers(void) {
+    char buf[FIXEDPT_STR_BUFSIZE];
+    fixedpt val = 123 << FIXEDPT_FBITS; // 123.0
+    size_t written;
+
+    // With 2 decimal places
+    written = fixedpt_str(val, buf, sizeof(buf), 2);
+    CU_ASSERT_STRING_EQUAL(buf, "123.00");
+    CU_ASSERT_EQUAL(written, 6);
+
+    // Trim trailing zeros
+    written = fixedpt_str(val, buf, sizeof(buf), -3);
+    CU_ASSERT_STRING_EQUAL(buf, "123");
+    CU_ASSERT_EQUAL(written, 3);
+}
+
+void test_negative_numbers(void) {
+    char buf[FIXEDPT_STR_BUFSIZE];
+    fixedpt val = -(456 << FIXEDPT_FBITS); // -456.0
+    size_t written;
+
+    written = fixedpt_str(val, buf, sizeof(buf), 2);
+    CU_ASSERT_STRING_EQUAL(buf, "-456.00");
+    CU_ASSERT_EQUAL(written, 7);
+}
+
+void test_exact_fractions(void) {
+    char buf[FIXEDPT_STR_BUFSIZE];
+    fixedpt val = FIXEDPT_ONE + FIXEDPT_ONE_HALF; // 1.5
+    size_t written;
+
+    // Normal precision
+    written = fixedpt_str(val, buf, sizeof(buf), 1);
+    CU_ASSERT_STRING_EQUAL(buf, "1.5");
+    CU_ASSERT_EQUAL(written, 3);
+
+    // Trim zeros
+    written = fixedpt_str(val, buf, sizeof(buf), -3);
+    CU_ASSERT_STRING_EQUAL(buf, "1.5");
+    CU_ASSERT_EQUAL(written, 3);
+}
+
+void test_buffer_limits(void) {
+    char small_buf[5];
+    fixedpt val = (123 << FIXEDPT_FBITS) + FIXEDPT_ONE_HALF; // 123.5
+    size_t written;
+
+    // Try to write to tiny buffer
+    written = fixedpt_str(val, small_buf, sizeof(small_buf), 2);
+    CU_ASSERT_STRING_EQUAL(small_buf, "123");
+    CU_ASSERT_EQUAL(written, 3);
+}
+
+void test_zero_trimming(void) {
+    char buf[FIXEDPT_STR_BUFSIZE];
+    fixedpt val = (123 << FIXEDPT_FBITS) + (FIXEDPT_ONE / 4); // 123.25
+    size_t written;
+
+    // Add extra zeros
+    written = fixedpt_str(val, buf, sizeof(buf), 4);
+    CU_ASSERT_STRING_EQUAL(buf, "123.2500");
+    CU_ASSERT_EQUAL(written, 8);
+
+    // Trim to significant digits
+    written = fixedpt_str(val, buf, sizeof(buf), -3);
+    CU_ASSERT_STRING_EQUAL(buf, "123.25");
+    CU_ASSERT_EQUAL(written, 6);
+}
+
+void test_max_precision(void) {
+    char buf[FIXEDPT_STR_BUFSIZE];
+    fixedpt val = FIXEDPT_ONE / 10; // 0.1
+    size_t written;
+
+    // Request maximum precision
+    written = fixedpt_str(val, buf, sizeof(buf), -2);
+    CU_ASSERT_STRING_EQUAL(buf, "0.097656250000000");
+    CU_ASSERT_EQUAL(written, 17);
+}
+
+void test_min_value(void) {
+    char buf[FIXEDPT_STR_BUFSIZE];
+    fixedpt val = FIXEDPT_MIN; // INT32_MIN >> 8
+    size_t written;
+
+    written = fixedpt_str(val, buf, sizeof(buf), 2);
+    CU_ASSERT_STRING_EQUAL(buf, "-8388608.00");
+    CU_ASSERT_EQUAL(written, 11);
+}
+
+void test_near_min_value(void) {
+    char buf[FIXEDPT_STR_BUFSIZE];
+    fixedpt val = FIXEDPT_MIN + (FIXEDPT_ONE / 4);
+    size_t written;
+
+    written = fixedpt_str(val, buf, sizeof(buf), 2);
+    CU_ASSERT_STRING_EQUAL(buf, "-8388607.75");
+    CU_ASSERT_EQUAL(written, 11);
+}
+
+void test_max_value(void) {
+    char buf[FIXEDPT_STR_BUFSIZE];
+    fixedpt val = FIXEDPT_MAX; // INT32_MAX >> 8
+    size_t written;
+
+    written = fixedpt_str(val, buf, sizeof(buf), 2);
+    CU_ASSERT_STRING_EQUAL(buf, "8388607.99");
+    CU_ASSERT_EQUAL(written, 10);
+}
+
+void test_near_max_value(void) {
+    char buf[FIXEDPT_STR_BUFSIZE];
+    fixedpt val = FIXEDPT_MAX - (FIXEDPT_ONE / 4);
+    size_t written;
+
+    written = fixedpt_str(val, buf, sizeof(buf), 2);
+    CU_ASSERT_STRING_EQUAL(buf, "8388607.74");
+    CU_ASSERT_EQUAL(written, 10);
+}
+
+void fixedpt_str_test_suite(CU_pSuite suite) {
+    CU_add_test(suite, "Zero Handling", test_zero_value);
+    CU_add_test(suite, "Positive Integers", test_positive_integers);
+    CU_add_test(suite, "Negative Numbers", test_negative_numbers);
+    CU_add_test(suite, "Exact Fractions", test_exact_fractions);
+    CU_add_test(suite, "Buffer Limits", test_buffer_limits);
+    CU_add_test(suite, "Zero Trimming", test_zero_trimming);
+    CU_add_test(suite, "Max Precision", test_max_precision);
+    CU_add_test(suite, "Min Value", test_min_value);
+    CU_add_test(suite, "Near Min Value", test_near_min_value);
+    CU_add_test(suite, "Max Value", test_max_value);
+    CU_add_test(suite, "Near Max Value", test_near_max_value);
+}