bpo-41798: Allocate unicodedata CAPI on the heap#24128
Uh oh!
There was an error while loading. Please reload this page.
Conversation
erlend-aasland commented Jan 5, 2021
cc @shihai1991 |
Modules/unicodedata.c Outdated
| PyMem_Free(capi); | ||
| return-1; | ||
| } | ||
| if (PyModule_AddObject(module, "_ucnhash_CAPI", capsule) <0){ |
There was a problem hiding this comment.
How about using PyModule_AddObjectRef in here too?
There was a problem hiding this comment.
We could do that, but I'm not sure it would improve this code. What do you think?
There was a problem hiding this comment.
PyModule_AddObjectRef() is more easy to understand than PyModule_AddObject(), because PyModule_AddObject() will steal the refs sometimes.
There was a problem hiding this comment.
After thinking about it, I do agree. It's easier to follow the ref count when reading the code. I'll change it. Thanks!
There was a problem hiding this comment.
Should I change the other PyModule_AddObject in unicodedata_exec() while we're there?
erlend-aasland commented Jan 7, 2021
@vstinner, would you mind reviewing this? |
vstinner commented Jan 8, 2021
Not yet. I'm fixing the second regression caused by your latest change :-D https://bugs.python.org/issue42866 I'm not blaming you, regressions are common, and I'm fine with dealing with them. It's just that I prefer to fix all known regressions before taking the risk of adding new ones ;-) |
erlend-aasland commented Jan 8, 2021
I totally understand that, and I appreciate you helping out fixing the regressions :) |
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
erlend-aasland commented Jan 20, 2021
PTAL, @vstinner |
vstinner commented Jan 20, 2021
Merged, thanks. |
erlend-aasland commented Jan 20, 2021
Thanks for reviewing! |
5 participants
https://bugs.python.org/issue41798