creachadair / imath

Arbitrary precision integer and rational arithmetic library
Other
131 stars 20 forks source link

mp_rat_read_cstring handles whitespace inconsistently #57

Open lhf opened 7 months ago

lhf commented 7 months ago

mp_rat_read_cstring handles whitespace inconsistently: it accepts " 123 / 46" but not " 123 ".

This happens because mp_int_read_cstring raises an error at trailing whitespace.

I assume mp_int_read_cstring tries to copy the behavior of strtol, but should it?

I suggest mp_int_read_cstring skip trailing whitespace.

Thanks for your work on imath.

creachadair commented 7 months ago

Hi @lhf, thanks for the report.

That is definitely a bug. 🙂 Specifically, "123 " was meant to be an error. I'd intended mp_rat_read_cstring to have the same behaviour as mp_int_read_cstring in that case, but I didn't back out correctly after probing for the / separator.

I can see a case for either being more liberal about spaces, or being less so. Since those functions (as you noted) are meant to mimic the API of strtol et al., I'm cautiously inclined to make them both report an error for extra spaces at the end.

That error is MP_TRUNC so the caller could still check for it, e.g.,

mp_result res = mp_rat_read_cstring(&v, str, 10, &endp);
if (res == MP_TRUNC) {
   while (isspace((unsigned char)*endp) { ++endp; }
   if (*endp != '\0') {
      return res;
   }
} else if (res != MP_OK) {
   return res;
}
// now v is valid and trailing spaces are consumed.

(probably in a little helper function somewhere, I guess)