FIX: SLEEF migration to 4.0 #240
Conversation
|
Took the opportunity to also clean the building process and warnings |
ngoldbaum
left a comment
ngoldbaum
left a comment
There was a problem hiding this comment.
It's unfortunate the parsing code had to change so much. It might be a little clearer if you do a first PR that does some refactoring in the parsing code, then rebased this PR on top of that one. Up to you if you feel like it's worth it.
I see you replaced cstring_to_quad with NumPyOS_ascii_strtoq - do we ever want to user the former anymore? If so, when? Maybe worth adding as a comment somewhere.
| // extracting absolute value | ||
| ix = hx & 0x7fffffffffffffffLL; | ||
| iy = hy & 0x7fffffffffffffffLL; | ||
| (void)ly; // unused but needed for quad_get_words64 |
There was a problem hiding this comment.
That just a read, so clang gives warnings, so just adding temporary void casting
| { | ||
| if(backend == BACKEND_SLEEF) { | ||
| out_value->sleef_value = Sleef_strtoq(str, endptr); | ||
| // SLEEF 4.0's Sleef_strtoq doesn't properly set endptr to indicate |
There was a problem hiding this comment.
kinda seems like a SLEEF bug you should report (even if they'll ignore it).
|
|
||
| // Call Sleef_strtoq with the bounded string | ||
| char *sleef_endptr; | ||
| out_value->sleef_value = Sleef_strtoq(temp, &sleef_endptr); |
There was a problem hiding this comment.
this is pretty subuptimal :(
Consider refactoring all the code you added here into a function even if there's only one caller, since it's pretty distracting here inline in this function.
|
|
||
| @pytest.mark.parametrize("invalid_bytes", [ | ||
| b"1.0 ", # Trailing whitespace | ||
| b"1.0 ", # Multiple trailing spaces |
There was a problem hiding this comment.
why delete test cases? it seems like the parsing code changed a lot above, if anything I'd expect added test cases here.
There was a problem hiding this comment.
Because even if for scalars, numpy and python handle the trailing whitespace correctly and parse the input
and here these are the tests that tests the invalid inputs (meant to be failed)
3 participants
closes #239
Saturday spend well! Some points to mention:
utilities.cand their corresponding calls have been modified because of the modified behaviour ofSleef_strtoqand handling of negative NaN.tlfloat43a0252), not setting tomasteras maybe in future more routine behaviour changes can break the dtype here (Ideally in future it'll be better to migrate the core changes from master if something fundamental breaks)QBLASstill uses them, I'll update them there and then we can maybe update here or keep it as it is