NotificationsYou must be signed in to change notification settings
Fork32k
Star67.2k

gh-129594: Remove redundant check on varargs in _PyArg_CheckPositional#129595

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

Jump to bottom

Open

eendebakpt wants to merge3 commits intopython:main

base:main

Choose a base branch

fromeendebakpt:_PyArg_CheckPositional

Open

gh-129594: Remove redundant check on varargs in _PyArg_CheckPositional#129595

eendebakpt wants to merge3 commits intopython:mainfromeendebakpt:_PyArg_CheckPositional

+1 −2

Conversation

Copy link

Contributor

eendebakpt commentedFeb 2, 2025•
edited by bedevere-appbot
Loading

Issue:Remove redundant check onvarargs in_PyArg_CheckPositional #129594

Remove redundant check on varargs in _PyArg_CheckPositional

77f5f81

eendebakpt requested a review fromericsnowcurrently as acode owner

February 2, 2025 19:52

bedevere-appbot added the awaiting review label

Feb 2, 2025

bedevere-appbot mentioned this pull request

Feb 2, 2025

Remove redundant check onvarargs in_PyArg_CheckPositional#129594

Open

picnixz added the skip news label

Feb 2, 2025

skirpichev approved these changes

Feb 2, 2025

View reviewed changes

bedevere-appbot added awaiting core review and removed awaiting review labels

Feb 2, 2025

Merge branch 'main' into _PyArg_CheckPositional

416726d

eendebakpt requested a review frompicnixz

April 4, 2025 13:42

Merge branch 'main' into _PyArg_CheckPositional

07c68bd

picnixz approved these changes

Apr 4, 2025

View reviewed changes

Copy link

Member

picnixz left a comment•
edited
Loading

Choose a reason for hiding this comment

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

This check is indeed unnecessary but can you transform it into an assertion? just so that we're sure we don't call the function incorrectly or is it impossible to add some assertion inside this?

bedevere-appbot added awaiting merge and removed awaiting core review labels

Apr 4, 2025

Copy link

ContributorAuthor

eendebakpt commentedApr 8, 2025

I think we could use an assertion within the macro. But if there is no reason to have it, it can only cause confusion.

The code in question was moved around a bit by Victor Stinner (only refactoring, functionality was not touched afaics).
The check was introduced inhttps://github.com/python/cpython/pull/18609/files (under a slightly different nameANY_VARARGS). From that PR it is not clear to me why it was added.@isidentical As PR author do you know why the check was added?