-
-
Notifications
You must be signed in to change notification settings - Fork 32.2k
bpo-44530: Add co_qualname field to PyCodeObject #26941
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
4548084
to
008cb5a
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for opening a PR @P403n1x87. Let's discuss this on the bpo issue, but at the very least this needs to:
- Be exposed to Python side
- Have tests
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a couple of questions for the reviewers
I have made the requested changes; please review again |
Thanks for making the requested changes! @pablogsal: please review the changes made to this pull request. |
Include/funcobject.h
Outdated
@@ -55,7 +55,6 @@ PyAPI_DATA(PyTypeObject) PyFunction_Type; | |||
#define PyFunction_Check(op) Py_IS_TYPE(op, &PyFunction_Type) | |||
|
|||
PyAPI_FUNC(PyObject *) PyFunction_New(PyObject *, PyObject *); | |||
PyAPI_FUNC(PyObject *) PyFunction_NewWithQualName(PyObject *, PyObject *, PyObject *); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unfortunately removing this is not allowed per PEP 387
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 I have now restored it.
The MAKE_FUNCTION opcode now needs to pop one less value from the TOS.
7a12d31
to
3f2c9e9
Compare
@@ -7243,7 +7242,6 @@ makecode(struct compiler *c, struct assembler *a, PyObject *constslist, | |||
PyObject *consts = NULL; | |||
PyObject *localsplusnames = NULL; | |||
PyObject *localspluskinds = NULL; | |||
PyObject *name = NULL; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was unused but was not reported by the compiler, probably because of the Py_XDECREF(name)
below
Misc/NEWS.d/next/C API/2021-06-28-23-44-47.bpo-44530.qij7YC.rst
Outdated
Show resolved
Hide resolved
Seems that you need to rebase and run |
yup, done! 🙂 |
🤖 New build scheduled with the buildbot fleet by @pablogsal for commit d5ec309 🤖 If you want to schedule another build, you need to add the ":hammer: test-with-buildbots" label again. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hummm, seems that the Windows buildbots are failing with this PR:
======================================================================
ERROR: test_gprof (test.test_tools.test_gprof2html.Gprof2htmlTests)
----------------------------------------------------------------------
Traceback (most recent call last):
File "C:\buildbot.python.org\pull_request.kloth-win64\build\lib\test\test_tools\test_gprof2html.py", line 16, in setUp
self.gprof = import_tool('gprof2html')
^^^^^^^^^^^^^^^^^^^^^^^^^
File "C:\buildbot.python.org\pull_request.kloth-win64\build\lib\test\test_tools\__init__.py", line 35, in import_tool
return importlib.import_module(toolname)
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "C:\buildbot.python.org\pull_request.kloth-win64\build\lib\importlib\__init__.py", line 126, in import_module
return _bootstrap._gcd_import(name[level:], package, level)
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "<frozen importlib._bootstrap>", line 1050, in _gcd_import
File "<frozen importlib._bootstrap>", line 1027, in _find_and_load
File "<frozen importlib._bootstrap>", line 1006, in _find_and_load_unlocked
File "<frozen importlib._bootstrap>", line 688, in _load_unlocked
File "<frozen importlib._bootstrap_external>", line 889, in exec_module
File "<frozen importlib._bootstrap_external>", line 1022, in get_code
File "<frozen importlib._bootstrap_external>", line 682, in _compile_bytecode
ValueError: bad marshal data (unknown type code)
Link to the builds:
https://buildbot.python.org/all/#/builders/577/builds/83
https://buildbot.python.org/all/#/builders/593/builds/87
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 |
Hmm is this related to the output of |
Ah, the problem is that we are missing bumping the magic number to avoid picking up old pyc files: cpython/Lib/importlib/_bootstrap_external.py Lines 212 to 374 in f64de53
|
I have made the requested changes; please review again |
Thanks for making the requested changes! @pablogsal: please review the changes made to this pull request. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 Looks good.
I'll leave merging to @pablogsal
Great job @P403n1x87! Happy to see Austin getting better :) |
https://bugs.python.org/issue44530
Sample flame graph for the script
Python 3.9
With proposed change