Codecov Report

Merging #18358 into master will decrease coverage by 0.04%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master   #18358      +/-   ##
==========================================
- Coverage   82.11%   82.07%   -0.05%     
==========================================
  Files        1955     1955              
  Lines      588624   584187    -4437     
  Branches    44406    44464      +58     
==========================================
- Hits       483366   479456    -3910     
+ Misses      95615    95110     -505     
+ Partials     9643     9621      -22     

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 850a4bd...f3a1d08. Read the comment docs.

Looks CI gate of macOS have broken, I saw some other PR have this problem.

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

I remove assert operation in get_locale_state(), becasue this assert operation always failed when I add traverse slot.
I am not check the code detail. From this result, removing the assert() from get_locale_state() is ok, because we have chance get A NULL state.

some error info like:

$ ./python -E -S -m sysconfig --generate-posix-vars
python: ./Modules/_localemodule.c:52: get_locale_state: Assertion `state != ((void *)0)' failed.
[1]    17894 abort      ./python -E -S -m sysconfig --generate-posix-vars

and the trace back:

#0  0x00007ffff6ce2207 in raise () from /lib64/libc.so.6
#1  0x00007ffff6ce38f8 in abort () from /lib64/libc.so.6
#2  0x00007ffff6cdb026 in __assert_fail_base () from /lib64/libc.so.6
#3  0x00007ffff6cdb0d2 in __assert_fail () from /lib64/libc.so.6
#4  0x00000000005986f3 in get_locale_state (m=<optimized out>) at ./Modules/_localemodule.c:52
#5  locale_traverse (m=<optimized out>, visit=0x462d26 <bad_traverse_test>, arg=0x0) at ./Modules/_localemodule.c:780
#6  0x000000000046384c in PyModule_FromDefAndSpec2TraceRefs (def=def@entry=0x96e680 <_localemodule>, spec=spec@entry=0x7ffff00567d0, module_api_version=module_api_version@entry=1013)
    at Objects/moduleobject.c:370
#7  0x0000000000512153 in _imp_create_builtin (module=<optimized out>, spec=0x7ffff00567d0) at Python/import.c:1300

I have made the requested changes; please review again.

Thanks for making the requested changes!

@encukou: please review the changes made to this pull request.

The assertion was good! If you remove it, you should add if (state != NULL) checks after any use of get_locale_state.
It's actually only the traverse/clear/free functions that can get a NULL module state. They can use PyModule_GetState with a NULL check directly.

I hope you don't mind me pushing to this branch directly – I already made the change and ran tests on it locally.

The assertion was good! If you remove it, you should add if (state != NULL) checks after any use of get_locale_state.
It's actually only the traverse/clear/free functions that can get a NULL module state. They can use PyModule_GetState with a NULL check directly.

I hope you don't mind me pushing to this branch directly – I already made the change and ran tests on it locally.

petr, pls go ahead ;)

The assertion was good! If you remove it, you should add if (state != NULL) checks after any use of get_locale_state.
It's actually only the traverse/clear/free functions that can get a NULL module state. They can use PyModule_GetState with a NULL check directly.

I hope you don't mind me pushing to this branch directly – I already made the change and ran tests on it locally.

Hi, petr. Do you have wins env to test this PR? #18608.
This' PR's failed info is weird. (Looks like module state have been confused~)