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

Fix limited API __Pyx_PyObject_ToDouble #5888

Merged
merged 9 commits into from Dec 3, 2023

Conversation

da-woods
Copy link
Contributor

@da-woods da-woods commented Dec 1, 2023

This fixes compilation of ExprNodes.py and Nodes.py and thus allows all Cython to be compiled with compile-more. (When combined with other limited API PRs)

This fixes compilation of ExprNodes.py and Nodes.py and thus
allows all Cython to be compiled with compile-more. (When
combined with other limited API PRs)
@da-woods da-woods added this to the 3.1 milestone Dec 1, 2023
Comment on lines 628 to 632
#if CYTHON_ASSUME_SAFE_MACROS
double value = PyFloat_AS_DOUBLE(float_value);
#else
double value = PyFloat_AsDouble(float_value);
#endif
Copy link
Contributor

Choose a reason for hiding this comment

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

We do this all over, so let's add a __Pyx_PyFloat_AS_DOUBLE() macro for this. That also avoids the need for a second special case in the __Pyx_PyObject_AsDouble() macro (which isn't as efficient as it could be since it doesn't dispatch to PyLong_AsDouble() directly (or even indirectly)).

Also, we should decide whether the right guard is "Limited API" or "safe macros". I don't think there is anything unsafe about calling PyFloat_AS_DOUBLE() on a known Python float object, so I vote for "Limited API" as a special case.

Cython/Utility/Optimize.c Outdated Show resolved Hide resolved
Co-authored-by: scoder <stefan_ml@behnel.de>
@scoder
Copy link
Contributor

scoder commented Dec 3, 2023 via email

@da-woods
Copy link
Contributor Author

da-woods commented Dec 3, 2023

I think we generally use an upper case Pyx for utility function names (and macros). Why did you lower case it?

Because that's how what two functions it was next to were. I've upper-cased them all now though

@scoder scoder merged commit bcc6799 into cython:master Dec 3, 2023
62 of 63 checks passed
@da-woods da-woods deleted the limited-api-obj-to-double branch December 3, 2023 19:34
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.

None yet

2 participants