Skip to content

Conversation

@SwayamInSync
Copy link
Member

This PR is a part of work, where making numpy_quaddtype compatible with NumPy's longdouble tests

Details

Use PyLong_AsLongLongAndOverflow instead of PyLong_AsLongLong, and handle the overflow case explicitly by reconstructing the QuadPrecision object via string conversion.

Copy link
Contributor

@juntyr juntyr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@ngoldbaum
Copy link
Member

I'd like to give the C code a careful once-over later today.

@SwayamInSync
Copy link
Member Author

I'd like to give the C code a careful once-over later today.

Cool, I'll do maybe 2 more PRs today (its a holiday here so xD)

Copy link
Member

@ngoldbaum ngoldbaum left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't spot any errors in the C code but I have some suggestions to improve code quality. In particular, it's good practice to test error cases if it's reasonable to do so and I spotted some opportunities for a refactor.

else if (lval == -1 && PyErr_Occurred()) {
Py_DECREF(py_int);
Py_DECREF(self);
return NULL;
Copy link
Member

@ngoldbaum ngoldbaum Oct 27, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you can test this error case by defining a class with an __index__ implementation that raises.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Somehow this isn't failing it, maybe need more low level of int corruption to trigger this.
Anyways rest are added

@SwayamInSync
Copy link
Member Author

Thanks @ngoldbaum mostly reviews seem to be connecting to more exhaustive testing, I'll soon add the tests to address them

@SwayamInSync
Copy link
Member Author

@ngoldbaum gentle reminder here, if this is GTG and also the #221
Last review was on mostly for test coverage on the error catches (they are added)

@ngoldbaum
Copy link
Member

Looks like we had a race condition. I just submitted the review for #221 and was going to look at this next.

Copy link
Member

@ngoldbaum ngoldbaum left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@ngoldbaum ngoldbaum merged commit 00fa954 into numpy:main Nov 4, 2025
11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants