cpython icon indicating copy to clipboard operation
cpython copied to clipboard

gh-94808: Remove unused code in _make_posargs

Open mjdominus opened this issue 1 year ago • 2 comments

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.

  • Issue: gh-94808

mjdominus avatar May 20 '24 16:05 mjdominus

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.

bedevere-app[bot] avatar May 20 '24 16:05 bedevere-app[bot]

@encukou

mjdominus avatar May 20 '24 16:05 mjdominus

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;

encukou avatar May 21 '24 16:05 encukou

I'd replace the branch with something like:

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

mjdominus avatar May 22 '24 00:05 mjdominus

All commit authors signed the Contributor License Agreement.
CLA signed

ghost avatar May 22 '24 06:05 ghost

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

encukou avatar May 22 '24 14:05 encukou

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

mjdominus avatar May 28 '24 14:05 mjdominus

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

mjdominus avatar Jun 04 '24 02:06 mjdominus

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 avatar Jun 04 '24 12:06 encukou

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

mjdominus avatar Jun 04 '24 13:06 mjdominus