Skip to content
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

bpo-45711: Remove type and traceback from exc_info #30122

Merged
merged 16 commits into from Dec 17, 2021

Conversation

iritkatriel
Copy link
Member

@iritkatriel iritkatriel commented Dec 15, 2021

@iritkatriel iritkatriel changed the title 45711 exc info just value bpo-45711: Remove type and traceback from exc_info Dec 15, 2021
@iritkatriel iritkatriel requested a review from gvanrossum Dec 15, 2021
@iritkatriel iritkatriel added the 🔨 test-with-buildbots Test the PR with the buildbot fleet and report in the status section label Dec 15, 2021
@bedevere-bot
Copy link

bedevere-bot commented Dec 15, 2021

🤖 New build scheduled with the buildbot fleet by @iritkatriel for commit 9b04160 🤖

If you want to schedule another build, you need to add the "🔨 test-with-buildbots" label again.

@bedevere-bot bedevere-bot removed the 🔨 test-with-buildbots Test the PR with the buildbot fleet and report in the status section label Dec 15, 2021
@@ -114,6 +108,10 @@ PyAPI_FUNC(void) _Py_NO_RETURN _Py_FatalRefcountErrorFunc(

#define _Py_FatalRefcountError(message) _Py_FatalRefcountErrorFunc(__func__, message)


/* number of items stored in the stack for each exception */
#define PY_EXC_INFO_STACK_SIZE 1
Copy link
Member

@markshannon markshannon Dec 16, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this necessary now?
I don't see why we would ever want to increase it, and I don't see how we could ever decrease it.

Copy link
Member Author

@iritkatriel iritkatriel Dec 16, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The name is probably weird now, but I think it helps readability to have a macro. If we remove this we should find another way to make the code (particularly in compile.c) clear about where exceptions are showing up. Figuring out where to use this macro was the hardest part of making this change (because when you get it wrong it just segfaults before you finish make regen-all).

Copy link
Member Author

@iritkatriel iritkatriel Dec 17, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On second thought there is no point in a macro for this unless we all agree on some kind of convention because it probably won't be updated correctly when people make changes. So I'll remove it.

Python/ceval.c Show resolved Hide resolved
@markshannon
Copy link
Member

markshannon commented Dec 16, 2021

This is great. 🍾

Removes code, should make any exception handling code faster and reduces stack consumption; the perfect combination.
A couple of very minor quibbles, otherwise looks really good.

Python/compile.c Outdated Show resolved Hide resolved
Co-authored-by: Oleg Iarygin <dralife@yandex.ru>
@iritkatriel
Copy link
Member Author

iritkatriel commented Dec 17, 2021

The windows test failure is unrelated

File "D:\a\cpython\cpython\Lib\test\support_init_.py", line 197, in _force_run
1232
return func(*args)
1233
^^^^^^^^^^^
1234
PermissionError: [WinError 32] The process cannot access the file because it is being used by another process: 'D:\a\cpython\cpython\build\test_python_4272�\test_python_worker_516�'

@markshannon
Copy link
Member

markshannon commented Dec 17, 2021

Excellent.

@markshannon markshannon merged commit 396b583 into python:main Dec 17, 2021
12 checks passed
@iritkatriel iritkatriel deleted the 45711-exc_info_just_value branch Jan 13, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants