Conversation
make default leaps seconds global constant and leap second array thread local
|
From afar, this approach seems sound to me, with the important limitation that the problem is entirely new to me and I don't know the internals of liberfa. I will defer any actual review to Marten. |
mhvk
left a comment
There was a problem hiding this comment.
This seems like a good change, but see my request for a comment about provenance in-line. In the meantime, I'll start the tests, just to check things do not break on something more obscure.
| ** For thread safety we want a way to define thread-local variables. | ||
| ** If no TLS is available, fall back to ordinary storage and disable | ||
| ** thread safety for those variables. | ||
| */ |
There was a problem hiding this comment.
It would be good to include in the comment where these definitions come from, in case things break later and we have to update or so. I notice that they are somewhat different from what is used by numpy
There was a problem hiding this comment.
I hadn't seen the numpy version, thanks for linking.
The differences in the conditions are that I missed C23 also has thread_local rather than _Thread_local and for cpp there is an explicit check of the version being > = C++11. I extended the comment to include more of this.
The main difference I see is the main checks are added in meson.build.
I tested this with erfa and it wasn't clear where this should go, I think for safety it would need to be in meson.build, configure.ac, and setup.py.
I'm happy to push a version with that, but it is quite a lot of repetition.
Do you think that would be better?
These are changes to enable use with freethreaded Python builds.
The only real change is making the leap second table (and count) thread local.
The logic for identifying how to find the appropriate identifier came from ChatGPT and I'm open to other suggestions.
This works with the OSs used in the CI and there is a fallback to the old behaviour.
See liberfa/pyerfa#209, cc @neutrinoceros @mhvk
I didn't implement the suggestion in #103 to preinitialize the leap second array.
It worked fine on mac/linux, but failed on windows (see this CI job).