FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

gh-100239: replace BINARY_SUBSCR & family by BINARY_OP with oparg NB_SUBSCR by iritkatriel · Pull Request #129700 · python/cpython · GitHub

/ cpython Public

gh-100239: replace BINARY_SUBSCR & family by BINARY_OP with oparg NB_SUBSCR - #129700

Merged
iritkatriel merged 17 commits into
python:mainfrom
iritkatriel:binary_subscr_to_op
Feb 7, 2025
ConversationCommits (17)ChecksFiles changed
Merged

gh-100239: replace BINARY_SUBSCR & family by BINARY_OP with oparg NB_SUBSCR#129700
iritkatriel merged 17 commits into
python:mainfrom
iritkatriel:binary_subscr_to_op

Conversation

iritkatriel commented Feb 5, 2025 •
edited by bedevere-app Bot
Loading

Copy link
Copy Markdown
Member

bedevere-app Bot mentioned this pull request Feb 5, 2025
Open
iritkatriel closed this Feb 5, 2025
iritkatriel reopened this Feb 5, 2025
iritkatriel mentioned this pull request Feb 5, 2025
Closed
iritkatriel commented Feb 5, 2025
View reviewed changes
Comment thread Python/specialize.c
Py_DECREF(descriptor);
return SPEC_FAIL_OTHER;
}
}

iritkatriel Feb 5, 2025

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

There is quite a bit or repetition between this function and _Py_Specialize_BinaryOp. I would like to merge them into one function, but not sure it should be in this PR.

@markshannon

markshannon Feb 7, 2025

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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Fix it in another PR.
I think it best to make the change in one PR and the refactoring in another.

markshannon approved these changes Feb 7, 2025
View reviewed changes

markshannon left a comment

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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Looks really good.

I'd like to check the stats before merging, to make sure all the specializations are still happening.

I'm not worried about the performance numbers; those will just be an artifact of code layout changes.

markshannon commented Feb 7, 2025

Copy link
Copy Markdown
Member

👍 The stats are as expected.

iritkatriel enabled auto-merge (squash) February 7, 2025 18:42
iritkatriel merged commit a1417b2 into python:main Feb 7, 2025
xuantengh mentioned this pull request Mar 15, 2025
Merged
tomasr8 mentioned this pull request Mar 15, 2025
Closed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

markshannon markshannon approved these changes
Fidget-Spinner Awaiting requested review from Fidget-Spinner Fidget-Spinner is a code owner
ericsnowcurrently Awaiting requested review from ericsnowcurrently ericsnowcurrently is a code owner

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

Back | FazBrowse Home | New Git URL