Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

gh-126072: do not addNone toco_consts if no docstring#126101

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged
markshannon merged 26 commits intopython:mainfromxuantengh:main
Oct 30, 2024

Conversation

@xuantengh
Copy link
Contributor

@xuantenghxuantengh commentedOct 29, 2024•
edited
Loading

Hi@markshannon this is my attempt to implement#126072. Initially the value ofCO_HAS_DOCSTRING is set as0x0040, but I found it has been used by theCO_NOFREE ininspect.

cpython/Lib/inspect.py

Lines 409 to 411 in85799f1

co_flags bitmap: 1=optimized | 2=newlocals | 4=*arg | 8=**arg
| 16=nested | 32=generator | 64=nofree | 128=coroutine
| 256=iterable_coroutine | 512=async_generator

And I've tested it with the following code: See the test cases in changed test files

I've also updated the related documentation, but maybe there is something I missed. Please help me improve this PR, thank you!

📚 Documentation preview 📚:https://cpython-previews--126101.org.readthedocs.build/

@bedevere-app
Copy link

Most changes to Pythonrequire a NEWS entry. Add one using theblurb_it web app or theblurb command-line tool.

If this change has little impact on Python users, wait for a maintainer to apply theskip news label instead.

Copy link
Member

@JelleZijlstraJelleZijlstra left a comment

Choose a reason for hiding this comment

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

Please add some tests, probably totest_code.py.

You'll also probably need to change the__doc__ descriptor on function objects so it continues to work correctly.

@xuantengh
Copy link
ContributorAuthor

xuantengh commentedOct 29, 2024•
edited
Loading

Maybe the optimization pass should be updated as the hypothesis may not hold:

cpython/Python/flowgraph.c

Lines 2117 to 2119 in9b14083

// The first constant may be docstring; keep it always.
index_map[0]=0;

The generated bytecodes for the same function are different before and after this PR.

@markshannon
Copy link
Member

FYI, we are in the process of removing small ints from theco_consts tuple:#125972

Can you change the ints in your tests to strings. Doing so will also help test that non-docstring strings do not get interpreted as docstrings.

@markshannon
Copy link
Member

Maybe the optimization pass should be updated as the hypothesis may not hold:

cpython/Python/flowgraph.c

Lines 2117 to 2119 in9b14083

// The first constant may be docstring; keep it always.
index_map[0]=0;

The generated bytecodes for the same function are different before and after this PR.

There are a few improvements we can make to the bytecode compiler once this PR is done.
It is probably best to do those in other PRs, once this is working.

@xuantengh
Copy link
ContributorAuthor

xuantengh commentedOct 29, 2024•
edited
Loading

It is probably best to do those in other PRs, once this is working.

Yeah, what I concern is whether the constant removal optimization will lead to a unexpected results when the first element inco_consts is no longerNone if there is no docstring.

@xuantengh
Copy link
ContributorAuthor

Currently, as the const removal optimization unconditionally keeps the first element in consts,None will present inco_consts even if co has no docstring if there is only one const (i.e.,None itself).

Copy link
Contributor

@willingcwillingc left a comment

Choose a reason for hiding this comment

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

Looks good. One small edit for the language reference.

Copy link
Member

@markshannonmarkshannon left a comment

Choose a reason for hiding this comment

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

Thanks for doing this.

This was one of those little things that's been bothering me for a while.
Glad to see it fixed

willingc and xuantengh reacted with thumbs up emoji
@xuantengh
Copy link
ContributorAuthor

Update docinspect.rst as well.

Copy link
Contributor

@willingcwillingc left a comment

Choose a reason for hiding this comment

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

One small comment on inspect.rst.

@markshannon
Copy link
Member

The UI shows 2 jobs still in progress. But they have completed successfully 🤷‍♂️

@markshannonmarkshannon merged commit35df4eb intopython:mainOct 30, 2024
39 checks passed
@xuantengh
Copy link
ContributorAuthor

Thanks for your review, it's my first PR merged into CPython! And I'm also interested in the following related optimizations.

iritkatriel and willingc reacted with hooray emoji

@iritkatriel
Copy link
Member

Thanks for your review, it's my first PR merged into CPython! And I'm also interested in the following related optimizations.

Congratulations! in your next PR, add your name toMisc/ACKS (we should have done it here, but too late now).

willingc reacted with thumbs up emoji

picnixz pushed a commit to picnixz/cpython that referenced this pull requestDec 8, 2024
ebonnal pushed a commit to ebonnal/cpython that referenced this pull requestJan 12, 2025
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@JelleZijlstraJelleZijlstraJelleZijlstra left review comments

@iritkatrieliritkatrieliritkatriel left review comments

@willingcwillingcwillingc approved these changes

@markshannonmarkshannonmarkshannon approved these changes

@carljmcarljmAwaiting requested review from carljmcarljm 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.

5 participants

@xuantengh@markshannon@iritkatriel@JelleZijlstra@willingc
[8]ページ先頭
©2009-2025 Movatter.jp