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-110964: Remove private _PyArg functions #110966

Merged
merged 2 commits into from
Oct 17, 2023

Conversation

vstinner
Copy link
Member

@vstinner vstinner commented Oct 17, 2023

Move the following private functions and structures to pycore_modsupport.h internal C API:

  • _PyArg_BadArgument()
  • _PyArg_CheckPositional()
  • _PyArg_NoKeywords()
  • _PyArg_NoPositional()
  • _PyArg_ParseStack()
  • _PyArg_ParseStackAndKeywords()
  • _PyArg_Parser structure
  • _PyArg_UnpackKeywords()
  • _PyArg_UnpackKeywordsWithVararg()
  • _PyArg_UnpackStack()
  • _Py_ANY_VARARGS()

Changes:

  • Python/getargs.h now includes pycore_modsupport.h to export functions.

  • clinic.py now adds pycore_modsupport.h when one of these functions is used.

  • Add pycore_modsupport.h includes when a C extension uses one of these functions.

  • Define Py_BUILD_CORE_MODULE in C extensions which now include directly or indirectly (via code generated by Argument Clinic) pycore_modsupport.h:

    • _csv
    • _curses_panel
    • _dbm
    • _gdbm
    • _multiprocessing.posixshmem
    • _sqlite.row
    • _statistics
    • grp
    • resource
    • syslog
  • _testcapi: bad_get() no longer uses METH_FASTCALL calling convention but METH_VARARGS. Replace _PyArg_UnpackStack() with PyArg_ParseTuple().

  • _testcapi: add PYTESTCAPI_NEED_INTERNAL_API macro which is defined by _testcapi sub-modules which need the internal C API (pycore_modsupport.h): exceptions.c, float.c, vectorcall.c, watchers.c.

  • Remove Include/cpython/modsupport.h header file. Include/modsupport.h no longer includes the removed header file.

@vstinner
Copy link
Member Author

This change is big because it changes 122 files generated by Argument Clinic:

$ git show --stat|grep 'clinic/.*.c.h'|wc -l
122

I prefer to write a single PR to move all private _PyArg functions at once, because each moved function touch many files generated by AC.

Copy link
Member

@AlexWaygood AlexWaygood left a comment

Choose a reason for hiding this comment

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

The AC changes look fine to me! (I'll leave the question of whether the changes are worth making to those more skilled in C.)

Move the following private functions and structures to
pycore_modsupport.h internal C API:

* _PyArg_BadArgument()
* _PyArg_CheckPositional()
* _PyArg_NoKeywords()
* _PyArg_NoPositional()
* _PyArg_ParseStack()
* _PyArg_ParseStackAndKeywords()
* _PyArg_Parser structure
* _PyArg_UnpackKeywords()
* _PyArg_UnpackKeywordsWithVararg()
* _PyArg_UnpackStack()
* _Py_ANY_VARARGS()

Changes:

* Python/getargs.h now includes pycore_modsupport.h to export
  functions.
* clinic.py now adds pycore_modsupport.h when one of these functions
  is used.
* Add pycore_modsupport.h includes when a C extension uses one of
  these functions.
* Define Py_BUILD_CORE_MODULE in C extensions which now include
  directly or indirectly (via code generated by Argument Clinic)
  pycore_modsupport.h:

  * _csv
  * _curses_panel
  * _dbm
  * _gdbm
  * _multiprocessing.posixshmem
  * _sqlite.row
  * _statistics
  * grp
  * resource
  * syslog

* _testcapi: bad_get() no longer uses METH_FASTCALL calling
  convention but METH_VARARGS. Replace _PyArg_UnpackStack() with
  PyArg_ParseTuple().
* _testcapi: add PYTESTCAPI_NEED_INTERNAL_API macro which is defined
  by _testcapi sub-modules which need the internal C API
  (pycore_modsupport.h): exceptions.c, float.c, vectorcall.c,
  watchers.c.
* Remove Include/cpython/modsupport.h header file.
  Include/modsupport.h no longer includes the removed header file.
* Fix mypy clinic.py
@vstinner
Copy link
Member Author

For clinic.py, I was lazy and used the clinic global variable in bad_argument(). The correct fix is to pass a clinic argument to add bad_argument() methods and calling sites. Problem: there are many. Second problem: I wrote such change many times, but I lost my work since the overall change was blocked for different reasons. Also, I tried to keep this PR as small as possible.

In short, clinic.py should be reworked later, once this change lands.

Comment on lines +3523 to +3524
if clinic is not None:
clinic.add_include('pycore_modsupport.h', '_PyArg_BadArgument()')
Copy link
Member

Choose a reason for hiding this comment

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

or perhaps this?

Suggested change
if clinic is not None:
clinic.add_include('pycore_modsupport.h', '_PyArg_BadArgument()')
assert clinic is not None
clinic.add_include('pycore_modsupport.h', '_PyArg_BadArgument()')

Copy link
Member Author

Choose a reason for hiding this comment

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

As explained in my previous comment, I plan to write a follow-up for this code. We should not use the global variable, but pass an argument which cannot be None.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, I agree that's the more principled action in the longer term, and I'm fine with using the easier solution for now. But this is the same number of lines, and I think makes it clearer that we never expect clinic to be None here (if it is None, something has gone horribly wrong :-)

Copy link
Member Author

Choose a reason for hiding this comment

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

PR to refactor this code, to avoid if clinic is not None: PR #110982.

@vstinner vstinner merged commit be5e8a0 into python:main Oct 17, 2023
@vstinner vstinner deleted the pycore_modsupport branch October 17, 2023 12:30
@vstinner
Copy link
Member Author

For clinic.py, I was lazy and used the clinic global variable in bad_argument().

I wrote PR #110984 for that.

@vstinner
Copy link
Member Author

I merged my PR. @AlexWaygood: thanks for reviewing clinic.py changes.

aisk pushed a commit to aisk/cpython that referenced this pull request Feb 11, 2024
Move the following private functions and structures to
pycore_modsupport.h internal C API:

* _PyArg_BadArgument()
* _PyArg_CheckPositional()
* _PyArg_NoKeywords()
* _PyArg_NoPositional()
* _PyArg_ParseStack()
* _PyArg_ParseStackAndKeywords()
* _PyArg_Parser structure
* _PyArg_UnpackKeywords()
* _PyArg_UnpackKeywordsWithVararg()
* _PyArg_UnpackStack()
* _Py_ANY_VARARGS()

Changes:

* Python/getargs.h now includes pycore_modsupport.h to export
  functions.
* clinic.py now adds pycore_modsupport.h when one of these functions
  is used.
* Add pycore_modsupport.h includes when a C extension uses one of
  these functions.
* Define Py_BUILD_CORE_MODULE in C extensions which now include
  directly or indirectly (via code generated by Argument Clinic)
  pycore_modsupport.h:

  * _csv
  * _curses_panel
  * _dbm
  * _gdbm
  * _multiprocessing.posixshmem
  * _sqlite.row
  * _statistics
  * grp
  * resource
  * syslog

* _testcapi: bad_get() no longer uses METH_FASTCALL calling
  convention but METH_VARARGS. Replace _PyArg_UnpackStack() with
  PyArg_ParseTuple().
* _testcapi: add PYTESTCAPI_NEED_INTERNAL_API macro which is defined
  by _testcapi sub-modules which need the internal C API
  (pycore_modsupport.h): exceptions.c, float.c, vectorcall.c,
  watchers.c.
* Remove Include/cpython/modsupport.h header file.
  Include/modsupport.h no longer includes the removed header file.
* Fix mypy clinic.py
hroncok added a commit to hroncok/pygame that referenced this pull request Jun 18, 2024
Glyphack pushed a commit to Glyphack/cpython that referenced this pull request Sep 2, 2024
Move the following private functions and structures to
pycore_modsupport.h internal C API:

* _PyArg_BadArgument()
* _PyArg_CheckPositional()
* _PyArg_NoKeywords()
* _PyArg_NoPositional()
* _PyArg_ParseStack()
* _PyArg_ParseStackAndKeywords()
* _PyArg_Parser structure
* _PyArg_UnpackKeywords()
* _PyArg_UnpackKeywordsWithVararg()
* _PyArg_UnpackStack()
* _Py_ANY_VARARGS()

Changes:

* Python/getargs.h now includes pycore_modsupport.h to export
  functions.
* clinic.py now adds pycore_modsupport.h when one of these functions
  is used.
* Add pycore_modsupport.h includes when a C extension uses one of
  these functions.
* Define Py_BUILD_CORE_MODULE in C extensions which now include
  directly or indirectly (via code generated by Argument Clinic)
  pycore_modsupport.h:

  * _csv
  * _curses_panel
  * _dbm
  * _gdbm
  * _multiprocessing.posixshmem
  * _sqlite.row
  * _statistics
  * grp
  * resource
  * syslog

* _testcapi: bad_get() no longer uses METH_FASTCALL calling
  convention but METH_VARARGS. Replace _PyArg_UnpackStack() with
  PyArg_ParseTuple().
* _testcapi: add PYTESTCAPI_NEED_INTERNAL_API macro which is defined
  by _testcapi sub-modules which need the internal C API
  (pycore_modsupport.h): exceptions.c, float.c, vectorcall.c,
  watchers.c.
* Remove Include/cpython/modsupport.h header file.
  Include/modsupport.h no longer includes the removed header file.
* Fix mypy clinic.py
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