Skip to content

GH-103963: Make dis display names of args for intrinsics opcodes - #104029

Merged
JelleZijlstra merged 15 commits into
python:mainfrom
jkchandalia:display_intrinsics_names
May 3, 2023
Merged

GH-103963: Make dis display names of args for intrinsics opcodes#104029
JelleZijlstra merged 15 commits into
python:mainfrom
jkchandalia:display_intrinsics_names

Conversation

@jkchandalia

@jkchandaliajkchandalia commented May 1, 2023•
edited by bedevere-bot
Loading

Copy link
Copy Markdown
Contributor

Make the dis module display the names of the args for opcodes CALL_INTRINSIC_*.

Fixes#103963

@bedevere-botbedevere-bot mentioned this pull request May 1, 2023
Closed
@jkchandalia

jkchandalia commented May 1, 2023

Copy link
Copy Markdown
ContributorAuthor

FYI @iritkatriel@nanjekyejoannah@JelleZijlstra

JelleZijlstra
JelleZijlstra reviewed May 1, 2023
View reviewed changes

@JelleZijlstraJelleZijlstra 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.

Looks good, a few comments. You'll also have to add a NEWS entry.

Comment threadLib/test/test_dis.py Outdated
Comment threadTools/build/generate_opcode_h.py Outdated
iritkatriel
iritkatriel reviewed May 1, 2023
View reviewed changes
Comment threadTools/build/generate_opcode_h.py Outdated
@jkchandalia

jkchandalia commented May 2, 2023

Copy link
Copy Markdown
ContributorAuthor

@iritkatriel@JelleZijlstra I've reviewed your comments and updated the PR. Please let me know if you see anything else or if I should convert to normal PR.

JelleZijlstra
JelleZijlstra reviewed May 2, 2023
View reviewed changes
Comment threadLib/test/test_dis.py Outdated
JelleZijlstra
JelleZijlstra approved these changes May 2, 2023
View reviewed changes

@JelleZijlstraJelleZijlstra 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.

Looks great!

@JelleZijlstra

JelleZijlstra commented May 2, 2023

Copy link
Copy Markdown
Member

I'll give Irit some time to take another look, but I think this is ready to merge.

@hugovkhugovk mentioned this pull request May 2, 2023
Closed
iritkatriel
iritkatriel reviewed May 2, 2023
View reviewed changes
Comment threadInclude/internal/pycore_intrinsics.h Outdated


/* Binary Functions: */
#define INTRINSIC_INVALID 0

@iritkatrieliritkatrielMay 2, 2023

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.

Looks like INTRINSIC_INVALID is defined twice?

@jkchandaliajkchandaliaMay 2, 2023

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Updated to differentiate between invalid for the two CALL_INTRINSIC_* opcodes.

@iritkatriel
iritkatriel marked this pull request as ready for review May 2, 2023 17:46
@JelleZijlstra
JelleZijlstra merged commit 872cbc6 into python:mainMay 3, 2023
@erlend-aaslanderlend-aasland mentioned this pull request May 3, 2023
Merged
@carljmcarljm mentioned this pull request May 4, 2023
Merged
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@JelleZijlstraJelleZijlstraJelleZijlstra approved these changes
@iritkatrieliritkatrieliritkatriel approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

dis: Show names of intrinsics

4 participants

@jkchandalia@JelleZijlstra@iritkatriel@bedevere-bot