Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

mp_rat_read_cstring handles whitespace inconsistently #57

Open
lhf opened this issue Mar 29, 2024 · 1 comment
Open

mp_rat_read_cstring handles whitespace inconsistently #57

lhf opened this issue Mar 29, 2024 · 1 comment
Assignees
Labels

Comments

@lhf
Copy link

lhf commented Mar 29, 2024

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
Copy link
Owner

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 creachadair self-assigned this Mar 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants