Skip to content

gh-101409: Improve generated clinic code for self type checks#101411

Merged
erlend-aasland merged 3 commits intopython:mainfrom
erlend-aasland:clinic/self-type-check
Jan 31, 2023
Merged

gh-101409: Improve generated clinic code for self type checks#101411
erlend-aasland merged 3 commits intopython:mainfrom
erlend-aasland:clinic/self-type-check

Conversation

@erlend-aasland
Copy link
Contributor

@erlend-aasland erlend-aasland commented Jan 29, 2023 •
edited by bedevere-bot
Loading

Store the self type pointer in a local variable.

@erlend-aasland
pythongh-101409: Improve generated clinic code for self type checks
056abc4 
Store the self type pointer in a local variable.
@bedevere-bot bedevere-bot mentioned this pull request Jan 29, 2023
Closed
@erlend-aasland
Copy link
Contributor Author

erlend-aasland commented Jan 29, 2023

cc. @colorfulappl

@erlend-aasland erlend-aasland mentioned this pull request Jan 29, 2023
Merged
@rhettinger rhettinger removed their request for review January 29, 2023 23:53
@colorfulappl
Copy link
Contributor

colorfulappl commented Jan 30, 2023

I'm not sure if this improvement is worth it, though; it may be churn for no benefit.

IMHO, this improvement is worth it.

  1. If the clinic_state() macro is fairly complicated, the compiler won't eliminate the redundant function invocations.

  2. The generated code after this patch is more concise and readable.

@erlend-aasland
Copy link
Contributor Author

erlend-aasland commented Jan 30, 2023

  1. If the clinic_state() macro is fairly complicated, the compiler won't eliminate the redundant function invocations.

That is true; we cannot guarantee that the compiler will eliminate the redundant calls.

  1. The generated code after this patch is more concise and readable.

I agree with this.

Thanks for the review, @colorfulappl; highly appreciated.

serhiy-storchaka
serhiy-storchaka reviewed Jan 30, 2023
View reviewed changes
Copy link
Member

@serhiy-storchaka serhiy-storchaka left a comment

Choose a reason for hiding this comment

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

self_tp may be not a good name. It is not a type of self. Maybe base_type or declared_type or something?

@erlend-aasland
Copy link
Contributor Author

erlend-aasland commented Jan 30, 2023

self_tp may be not a good name. It is not a type of self. Maybe base_type or declared_type or something?

base_type sounds good; thanks!

@erlend-aasland
Copy link
Contributor Author

erlend-aasland commented Jan 31, 2023 •
edited
Loading

I'm considering landing this PR in a day or two. With three thumbs up, I'm landing this tonight.

kumaraditya303
kumaraditya303 approved these changes Jan 31, 2023
View reviewed changes
Copy link
Contributor

@kumaraditya303 kumaraditya303 left a comment

Choose a reason for hiding this comment

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

LGTM

@erlend-aasland erlend-aasland merged commit 2753cf2 into python:main Jan 31, 2023
@erlend-aasland erlend-aasland deleted the clinic/self-type-check branch January 31, 2023 20:42
@erlend-aasland
Copy link
Contributor Author

erlend-aasland commented Jan 31, 2023

Thanks for the reviews, y'all!

carljm added a commit to carljm/cpython that referenced this pull request Jan 31, 2023
@carljm
Merge branch 'main' into inlinecomp2
db208d5 
* main:
  pythonGH-100288: Skip extra work when failing to specialize LOAD_ATTR (pythonGH-101354)
  pythongh-101409: Improve generated clinic code for self type checks (python#101411)
  pythongh-98831: rewrite BEFORE_ASYNC_WITH and END_ASYNC_FOR in the instruction definition DSL (python#101458)
  pythongh-101469: Optimise get_io_state() by using _PyModule_GetState() (pythonGH-101470)
@erlend-aasland erlend-aasland mentioned this pull request Feb 3, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@serhiy-storchaka serhiy-storchaka serhiy-storchaka approved these changes

@kumaraditya303 kumaraditya303 kumaraditya303 approved these changes

@ethanfurman ethanfurman Awaiting requested review from ethanfurman

@berkerpeksag berkerpeksag Awaiting requested review from berkerpeksag

Assignees

No one assigned

Labels

skip news

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@erlend-aasland @colorfulappl @serhiy-storchaka @kumaraditya303 @bedevere-bot