Skip to content
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

gh-94808: Remove unused code in _make_posargs #119227

Merged
merged 2 commits into from
Jun 4, 2024
Merged

Conversation

mjdominus
Copy link
Contributor

@mjdominus mjdominus commented May 20, 2024

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
Copy link

bedevere-app bot commented May 20, 2024

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.

@mjdominus
Copy link
Contributor Author

@encukou

@encukou
Copy link
Member

encukou commented May 21, 2024

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;

@mjdominus
Copy link
Contributor Author

I'd replace the branch with something like:

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

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

cpython-cla-bot bot commented May 22, 2024

All commit authors signed the Contributor License Agreement.
CLA signed

@mjdominus mjdominus marked this pull request as draft May 22, 2024 07:32
@encukou
Copy link
Member

encukou commented May 22, 2024

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

@mjdominus
Copy link
Contributor Author

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

`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
Copy link
Contributor Author

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

@encukou
Copy link
Member

encukou commented Jun 4, 2024

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 encukou merged commit bd8c1f9 into python:main Jun 4, 2024
35 checks passed
@mjdominus mjdominus deleted the mjd2 branch June 4, 2024 13:56
@mjdominus
Copy link
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.
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