bpo-1635741: Convert _sre types to heap types and establish module state (PEP 384) by erlend-aasland · Pull Request #23393 · python/cpython

erlend-aasland · 2020-11-19T09:03:03Z

https://bugs.python.org/issue1635741

erlend-aasland · 2020-11-19T09:05:18Z

@vstinner / @corona10: Would you mind reviewing this?

tiran

See inline comments

While you are working on the module, please remove PyModule_GetDict() and replace the code with PyModule_AddStringConstant().

bedevere-bot · 2020-11-19T09:13:28Z

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.

erlend-aasland · 2020-11-19T09:16:22Z

See inline comments
While you are working on the module, please remove PyModule_GetDict() and replace the code with PyModule_AddStringConstant().

I could merge #23101 into this PR. Would that be acceptable?

tiran · 2020-11-19T09:20:28Z

See inline comments
While you are working on the module, please remove PyModule_GetDict() and replace the code with PyModule_AddStringConstant().
I could merge #23101 into this PR. Would that be acceptable?

+1, good idea

You'll get bonus points for using PyModule_AddObjectRef.

erlend-aasland · 2020-11-19T09:38:13Z

You'll get bonus points for using PyModule_AddObjectRef.

GH-23101 merged with commit 38e3cd7.

from Lib/idlelib/idle_test/test_calltip.py: The tests of builtins may break if inspect or the docstrings change, but a red buildbot is better than a user crash (as has happened). For a simple mismatch, change the expected output to the actual.

erlend-aasland · 2020-11-19T12:11:22Z

I have made the requested changes; please review again

bedevere-bot · 2020-11-19T12:11:25Z

Thanks for making the requested changes!

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

tiran

Thanks!

I'm leaving the final ack to @corona10 and @vstinner

corona10

lgtm awesome,

I ran manually some tests and works well.

corona10

But I want to know why this behavior is changed.

master

>>>importre>>>p=re.compile('')
>>>p.sub.__doc__'Return the string obtained by replacing the leftmost non-overlapping occurrences of pattern in string by the replacement repl.'

PR

>>>importre>>>p=re.compile('')
>>>p.sub.__doc__None

pending

erlend-aasland · 2020-11-19T13:20:18Z

But I want to know why this behavior is changed.

I find that odd as well. ~~At first, I though it was just the AC that messed up the argument list, but it's not.~~ The docstring is clearly there in line 1214. Try this:

>>> import re
>>> p = re.compile('')
>>> help(p.sub)

It seems to happen only with methods with the defining_class converter. Could it be a bug in AC?

UPDATE

Seems like dbm has the same problem. For example, the _dbm.dbm.keys method, which also has a defining_class converter, and is a method defined on a type belonging to the module:

>>> import dbm
>>> d = dbm.open('test.db', 'c')
>>> d
<_gdbm.gdbm object at 0x103eeb500>
>>> d.keys.__doc__ # <= this returns None iso. the docstring as defined in AC
>>> help(d.keys) # <= this displays the docstring as defined in AC

This reverts commit a6bc481.

erlend-aasland · 2020-11-19T18:42:45Z

@corona10 or @tiran can you remove Terry J. Reedy as reviewer? Adjusting the idlelib test was a bad idea. Dong-hee Na is right; there's something fishy about the __doc__ attributes for some of the methods.

bedevere-bot · 2020-11-19T19:43:34Z

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.

terryjreedy · 2020-11-20T07:03:43Z

Actually, tests should pass now because test runners will use revised idlelib.

terryjreedy · 2020-11-20T07:35:49Z

@tiran The IDLE test failure is gone. Do you still want changes?

erlend-aasland · 2020-11-20T07:59:25Z

@corona10 IDLE has been fixed, so tests pass fine now. I'll file an issue about the missing __doc__ strings in builtin methods.

corona10

lgtm

…ate (PEP 384) (pythonGH-23393)

Erlend E. Aasland added 8 commits November 19, 2020 09:55

Add convenience macro
78b9d53

Convert _sre pattern type to heap type
ce746b3

Convert _sre match type to heap type
529142d

Convert _sre scanner type to heap type
7583104

Establish global module state and add types to it
575fccf

Convert global state to module state
f82463c

Convert _sre to multi-phase initialisation
63ad0f1

Add NEWS
27134f0

the-knights-who-say-ni added the CLA signed label Nov 19, 2020

bedevere-bot added the awaiting review label Nov 19, 2020

tiran requested changes Nov 19, 2020
View reviewed changes

Comment threadModules/_sre.c Outdated
Comment threadModules/_sre.c Outdated

bedevere-bot removed the awaiting review label Nov 19, 2020

bedevere-bot added the awaiting changes label Nov 19, 2020

Erlend E. Aasland added 3 commits November 19, 2020 10:54

Address Heimes' review: improve module state naming
1c925b9

MergepythonGH-23101
12bf771

Update clinic
0f10f7d

erlend-aasland force-pushed the bpo-1635741/sre branch from 2aaa00a to 0f10f7dCompare November 19, 2020 09:55

Address Heimes' review: Drop using _PyType_GetModuleByDef
bdd1338

erlend-aasland force-pushed the bpo-1635741/sre branch from 6e6ab04 to bdd1338Compare November 19, 2020 10:18

Adjust idlelib tiptest
a6bc481
from Lib/idlelib/idle_test/test_calltip.py: The tests of builtins may break if inspect or the docstrings change, but a red buildbot is better than a user crash (as has happened). For a simple mismatch, change the expected output to the actual.

erlend-aasland requested a review from terryjreedy as a code owner November 19, 2020 11:19

bedevere-bot added awaiting change review and removed awaiting changes labels Nov 19, 2020

tiran reviewed Nov 19, 2020
View reviewed changes

corona10 previously approved these changes Nov 19, 2020
View reviewed changes

bedevere-bot added awaiting merge and removed awaiting change review labels Nov 19, 2020

corona10 reviewed Nov 19, 2020
View reviewed changes

Revert "Adjust idlelib tiptest"
2fd3bd1
This reverts commit a6bc481.

terryjreedy requested changes Nov 19, 2020
View reviewed changes

Comment threadLib/idlelib/idle_test/test_calltip.py Outdated

bedevere-bot removed the awaiting merge label Nov 19, 2020

bedevere-bot added the awaiting changes label Nov 19, 2020

terryjreedy closed this Nov 20, 2020

terryjreedy reopened this Nov 20, 2020

terryjreedy approved these changes Nov 20, 2020
View reviewed changes

bedevere-bot added awaiting merge and removed awaiting changes labels Nov 20, 2020

Merge branch 'master' into bpo-1635741/sre
d9ca7a6

corona10 approved these changes Nov 20, 2020
View reviewed changes

corona10 merged commit a6109ef into python:masterNov 20, 2020

bedevere-bot removed the awaiting merge label Nov 20, 2020

erlend-aasland mentioned this pull request Nov 20, 2020
bpo-42243: Use PyModule_Add*() in _sre iso. direct module dict manipulation #23101
Closed

erlend-aasland deleted the bpo-1635741/sre branch November 20, 2020 15:02

adorilson pushed a commit to adorilson/cpython that referenced this pull request Mar 13, 2021
bpo-1635741: Convert _sre types to heap types and establish module st…
acdf30c
…ate (PEP 384) (pythonGH-23393)

serhiy-storchaka mentioned this pull request Apr 15, 2022
Regression in the AC generated code #91583
Closed

kylotanmannequin mentioned this pull request Sep 19, 2022
Py_Finalize() doesn't clear all Python objects at exit #44470
Closed

Uh oh!

Uh oh!

Conversation

erlend-aasland commented Nov 19, 2020• edited by bedevere-bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

erlend-aasland commented Nov 19, 2020

Uh oh!

tiran left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

bedevere-bot commented Nov 19, 2020

Uh oh!

erlend-aasland commented Nov 19, 2020

Uh oh!

tiran commented Nov 19, 2020

Uh oh!

erlend-aasland commented Nov 19, 2020

Uh oh!

erlend-aasland commented Nov 19, 2020

Uh oh!

bedevere-bot commented Nov 19, 2020

Uh oh!

tiran left a comment

Choose a reason for hiding this comment

Uh oh!

corona10 left a comment

Choose a reason for hiding this comment

Uh oh!

corona10 left a comment

Choose a reason for hiding this comment

master

PR

Uh oh!

erlend-aasland commented Nov 19, 2020• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

erlend-aasland commented Nov 19, 2020

Uh oh!

Uh oh!

bedevere-bot commented Nov 19, 2020

Uh oh!

terryjreedy commented Nov 20, 2020

Uh oh!

terryjreedy commented Nov 20, 2020

Uh oh!

erlend-aasland commented Nov 20, 2020

Uh oh!

corona10 left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

erlend-aasland commented Nov 19, 2020•
edited by bedevere-bot
Loading

erlend-aasland commented Nov 19, 2020•
edited
Loading