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

MAINT: Make keyword arrays static #18161

Merged
merged 1 commit into from
Feb 2, 2021
Merged

MAINT: Make keyword arrays static #18161

merged 1 commit into from
Feb 2, 2021

Conversation

alexhenrie
Copy link
Contributor

These arrays never change, and making them static will avoid having to reinitialize them on every function call.

@eric-wieser
Copy link
Member

I'm not convinced by this change - isn't it sufficient to mark them const?

@alexhenrie
Copy link
Contributor Author

Sadly, marking them const produces compiler warnings because PyArg_ParseTupleAndKeywords takes a non-const array even though it doesn't actually modify the array. git grep -F 'char *kwlist[]' shows that most of these arrays were already marked static in NumPy, and none are marked const. I'm just proposing that we be consistent and add static to the ones that didn't have it already.

@eric-wieser
Copy link
Member

I just concluded the same thing. The approach seems odd, but it's even how CPython itself does it. I'll maybe play around with godbolt to see if const is ultimately better, but at the end of the day consistency matters more than microoptimization here.

@eric-wieser
Copy link
Member

takes a non-const array even though it doesn't actually modify the array

As far as I remember this is due to a shortcoming in C (fixed in C++), where a T const* const argument cannot legally be passed a T*.

@seberg
Copy link
Member

seberg commented Feb 2, 2021

Sorry, forgot about these. Adding static uniformly seems great to me (some of this might cause me a merge conflict, but that is trivial). Thanks @alexhenrie.

@seberg seberg merged commit b449183 into numpy:master Feb 2, 2021
@alexhenrie
Copy link
Contributor Author

Thank you!

@alexhenrie alexhenrie deleted the static_kwlist branch February 2, 2021 20:06
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

3 participants