Skip to content

Conversation

LecrisUT
Copy link
Collaborator

@LecrisUT LecrisUT commented Jun 6, 2023

Are there other static variables that need to be changed?

TODO:

  • Test if it does what it says. If it can be done locally, that would be ok as well.
    Maybe a way to do this is to spawn a few thousands workers writing random values to spglib_error_code and re-reading after a delay to confirm it's the same value.
  • Check this compiles on intel compiler

@LecrisUT LecrisUT marked this pull request as draft June 6, 2023 13:07
@LecrisUT LecrisUT requested review from atztogo and lan496 June 6, 2023 14:16
@LecrisUT
Copy link
Collaborator Author

LecrisUT commented Jun 6, 2023

@atztogo That was a quick simple fix. Added a simple tester for it as well. (try to remove _Thread_local and it should fail as expected)

@LecrisUT LecrisUT added the bug label Jun 6, 2023
@atztogo
Copy link
Collaborator

atztogo commented Jun 7, 2023

Thanks @LecrisUT. This is interesting. I didn't know about _Thread_local al all. Can it be ignored when using older standard than C11? For many years, spglib had gotten stuck roughly C89 for supporting old microsoft c compiler. Now I don't think we need to support C89, but it made me conservative, and I became to tend to be afraid of new standard. I think what at least we should have to support are the compilers used in conda-forge spglib builds. So I believe there is no issue. Do you have any comment?

@LecrisUT
Copy link
Collaborator Author

LecrisUT commented Jun 7, 2023

So there is also __thread for older gcc compilers. But I don't think it is worth keeping the old standard especially since you can't compile those compilers on currently uaed distros.

I would say a good support track is to stick to what we can check through CI, until there is an issue created for older standard support. So, a common support cycle for this is: conda-forge + containered os (currently Fedora), which usually supports up to 2 previous major releases of the compilers.

If we want an older compiler support, we can make a specific CI job for that based on Ubuntu. But we should state an EOL support for it if we do so.

LecrisUT added 2 commits June 8, 2023 10:40
Signed-off-by: Cristian Le <cristian.le@mpsd.mpg.de>
Signed-off-by: Cristian Le <cristian.le@mpsd.mpg.de>
@LecrisUT LecrisUT marked this pull request as ready for review June 8, 2023 08:41
@LecrisUT LecrisUT merged commit 5905a36 into spglib:develop Jun 8, 2023
@LecrisUT LecrisUT deleted the thread-safety branch June 8, 2023 09:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants