Skip to content

[3.12] gh-124785: Revert "gh-116510: Fix crash due to shared immortal interned strings (gh-124646)" (gh-124807) - #124818

Closed
Yhg1s wants to merge 1 commit into
python:3.12from
Yhg1s:backport-7bdfabe-3.12
Closed

[3.12] gh-124785: Revert "gh-116510: Fix crash due to shared immortal interned strings (gh-124646)" (gh-124807)#124818
Yhg1s wants to merge 1 commit into
python:3.12from
Yhg1s:backport-7bdfabe-3.12

Conversation

@Yhg1s

@Yhg1sYhg1s commented Oct 1, 2024

Copy link
Copy Markdown
Member

Revert "gh-116510: Fix crash due to shared immortal interned strings. (gh-124646)"

This reverts commit 98b2ed7.
(cherry picked from commit 7bdfabe)

…red immortal interned strings (pythongh-124646)" (pythongh-124807)
Revert "pythongh-116510: Fix crash due to shared immortal interned strings. (pythongh-124646)"
This reverts commit 98b2ed7.
(cherry picked from commit 7bdfabe)
Co-authored-by: T. Wouters <thomas@python.org>
@Yhg1s

Yhg1s commented Oct 1, 2024

Copy link
Copy Markdown
MemberAuthor

Already resolved in #124814.

@Yhg1sYhg1s closed this Oct 1, 2024
if (interned == NULL) {
return -1;
}
PyObject *interned = interned = PyDict_New();

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Why do you want to do this? Rather than

PyObject*interned=PyDict_New();

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

It's just a straight reversion of my change so that matches the previous code. I agree with you though, looks odd to write it that way.

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

It's a revert. Even worse, it's a backport of a revert. That's what the original code was. I imagine it was a mistake in the original PR (#102339) but that, too, may have been moved from somewhere else.

If you want to send a PR to fix it (in main) I'm sure Eric wouldn't mind reviewing :)

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

@Yhg1s@nascheme@rruuaanng