imath icon indicating copy to clipboard operation
imath copied to clipboard

mp_rat_read_cstring handles whitespace inconsistently

Open lhf opened this issue 10 months ago • 1 comments

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.

lhf avatar Mar 29 '24 10:03 lhf

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)

creachadair avatar Mar 30 '24 00:03 creachadair