← 返回首页
gh-94808: Remove unused code in _make_posargs by mjdominus · Pull Request #119227 · python/cpython · GitHub
Skip to content

Navigation Menu

Toggle navigation
Sign in
Appearance settings
Search or jump to...

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

Provide feedback

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

Include my email address so I can be contacted

Saved searches

Use saved searches to filter your results more quickly

Appearance settings
Resetting focus

gh-94808: Remove unused code in _make_posargs#119227

Merged
encukou merged 2 commits into
python:mainfrom
mjdominus:mjd2
Jun 4, 2024
Merged

gh-94808: Remove unused code in _make_posargs#119227
encukou merged 2 commits into
python:mainfrom
mjdominus:mjd2

Conversation

mjdominus commented May 20, 2024
edited
Loading

Copy link
Copy Markdown
Contributor

The coverage report indicates that this branch is never exercised. That appears to be because it is unused code. When I couldn't produce a test case that exercised it, I removed the branch, then checked that the complete test suite still passed.

The branch is only run when names_with_default is null. It appears that when there are no names with defaults, names_with_defaults will point to a structure that has .size zero.

It's possible that a future implementation change might alter that, requiring the branch to be put back, but I believe the existing tests would detect if that happened.

bedevere-app Bot commented May 20, 2024

Copy link
Copy Markdown

Most changes to Python require a NEWS entry. Add one using the blurb_it web app or the blurb command-line tool.

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

Copy link
Copy Markdown
Contributor Author

@encukou

encukou commented May 21, 2024

Copy link
Copy Markdown
Member

The current code covers all combinations of plain_names & names_with_default being NULL and non-NULL. Removing one of the cases breaks that, leaving no hint about why this would fall through to the empty posargs case.
If it was me, I'd replace the branch with something like:

// With the current grammar, we never get here. // When that changes, remove the assert, and test thoroughly. assert(0); *posargs = plain_names;

Copy link
Copy Markdown
Contributor Author

I'd replace the branch with something like:

I agree, that's better. Should I resubmit?

mjdominus force-pushed the mjd2 branch 2 times, most recently from 4b8f4ac to 4a15e0a Compare May 22, 2024 06:50

ghost commented May 22, 2024
edited by ghost
Loading

Copy link
Copy Markdown

All commit authors signed the Contributor License Agreement.

mjdominus marked this pull request as draft May 22, 2024 07:32

encukou commented May 22, 2024

Copy link
Copy Markdown
Member

This looks good to me, thanks!
I see you've marked it as draft, do you still want to make changes?

Copy link
Copy Markdown
Contributor Author

Thanks! I meant to wrap this up over the weekend, but forgot. I will do it this week.

mjdominus added 2 commits June 3, 2024 22:01
`names_with_default` is never `NULL`, even if there are no names with defaults. In that case it points to a structure with `size` zero. Rather than eliminating the branch, we leave it behind with an `assert(0)` in case a future change to the grammar exercises the branch.
mjdominus marked this pull request as ready for review June 4, 2024 02:06

Copy link
Copy Markdown
Contributor Author

@encukou I split the changes into two commits, one to improve coverage and the other to reorganize the if-else structure.

encukou commented Jun 4, 2024

Copy link
Copy Markdown
Member

Thanks!

For the future: please avoid force-pushing to CPython -- when reviewed commits disappear, the review needs to be restarted. (In this commit it's not a problem, as it's just a few lines.)
Reorganizing the commits doesn't help, since all PRs are squashed when merging.

encukou merged commit bd8c1f9 into python:main Jun 4, 2024
mjdominus deleted the mjd2 branch June 4, 2024 13:56

Copy link
Copy Markdown
Contributor Author

Thanks again for your help and guidance through the whole process.

barneygale pushed a commit to barneygale/cpython that referenced this pull request Jun 5, 2024
…GH-119227) * Reorganize four-way if-elsif-elsif-elsif as nested if-elses * Mark unused branch in _make_posargs `names_with_default` is never `NULL`, even if there are no names with defaults. In that case it points to a structure with `size` zero. Rather than eliminating the branch, we leave it behind with an `assert(0)` in case a future change to the grammar exercises the branch.
noahbkim pushed a commit to hudson-trading/cpython that referenced this pull request Jul 11, 2024
…GH-119227) * Reorganize four-way if-elsif-elsif-elsif as nested if-elses * Mark unused branch in _make_posargs `names_with_default` is never `NULL`, even if there are no names with defaults. In that case it points to a structure with `size` zero. Rather than eliminating the branch, we leave it behind with an `assert(0)` in case a future change to the grammar exercises the branch.
estyxx pushed a commit to estyxx/cpython that referenced this pull request Jul 17, 2024
…GH-119227) * Reorganize four-way if-elsif-elsif-elsif as nested if-elses * Mark unused branch in _make_posargs `names_with_default` is never `NULL`, even if there are no names with defaults. In that case it points to a structure with `size` zero. Rather than eliminating the branch, we leave it behind with an `assert(0)` in case a future change to the grammar exercises the branch.
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
Sign up for free to 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.

2 participants

Footer

© 2026 GitHub, Inc.