GeezerVison

The World Wide Web, restored to 800 pixels of glory.

GeezerVison is showing github.com — gh-65210: Add const qualifiers in PyArg_VaParseTupleAndKeywords() by serhiy-storchaka · Pull Request #105958 · python/cpython

Conversation

serhiy-storchaka commented Jun 21, 2023 •

edited by github-actions Bot

Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link

Copy Markdown

Member

Change the declaration of the keywords parameter in functions PyArg_ParseTupleAndKeywords() and PyArg_VaParseTupleAndKeywords() from char ** to char * const * in C and const char * const * in C++.

It makes these functions compatible with argument of type const char * const *, const char ** or char * const * in C++ and char * const * in C without explicit type cast.

Issue: PyArg_ParseTupleAndKeywords doesn't take const char *keywords[] #65210

📚 Documentation preview 📚: https://cpython-previews--105958.org.readthedocs.build/

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

arhadthedev and lpsinger reacted with thumbs up emoji

All reactions

👍 2 reactions


          pythongh-65210: Add const qualifiers in PyArg_VaParseTupleAndKeywords()

80e37a9

Change the declaration of the keywords parameter in functions
PyArg_ParseTupleAndKeywords() and PyArg_VaParseTupleAndKeywords() from `char **`
to `char * const *` in C and `const char * const *` in C++.

It makes these functions compatible with argument of type `const char * const *`,
`const char **` or `char * const *` in C++ and `char * const *` in C
without explicit type cast.

bedevere-bot added the awaiting core review label

Jun 21, 2023

bedevere-bot mentioned this pull request

Jun 21, 2023

PyArg_ParseTupleAndKeywords doesn't take const char *keywords[] #65210

Closed

serhiy-storchaka added the topic-C-API label

Jun 21, 2023


          Fix doc build.

hugovk reviewed

Jun 22, 2023

View reviewed changes

Comment thread

Doc/c-api/arg.rst Outdated


		The keywords parameter declaration is :c:expr:`char * const *` in C and
		:c:expr:`const char * const *` in C++.
		This can be overridden by defining the macro :c:macro:`PY_CXX_CONST`

hugovk Jun 22, 2023

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Please document this new macro somewhere with .. c:macro:: PY_CXX_CONST.

That will fix this warning and the one in 3.13.rst and the CI.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

serhiy-storchaka Jun 22, 2023

Copy link

Copy Markdown

Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

I do not know how to document it better than I did. I will appreciate your suggestions. For now I just removed the role.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

hugovk Jun 24, 2023

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

@CAM-Gerlach Any suggestions?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

CAM-Gerlach Jun 24, 2023

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

@serhiy-storchaka To properly document a public entity (macro, function, class, method, etc), you can use the the appropriate directive—in this case, .. c:macro:: as @hugovk mentioned. This could be something like (placed, perhaps, at the top or bottom of the API Functions subsection):

.. macro:: PY_CXX_CONST

   The value to be inserted, if any, before :c:expr:`char * const *`
   in the *keywords* parameter declaration of
   :c:func:`PyArg_ParseTupleAndKeywords`
   and :c:func:`PyArg_VaParseTupleAndKeywords`.
   Default empty for C and ``const`` for C++ (:c:expr:`const char * const *`).
   To override, define it to the desired value before including :file:`Python.h`.

   .. versionadded:: 3.13

You can then simplify this note accordingly, per my other suggestion (in a separate comment).

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions


          Silence Sphinx warning.

040817e

arhadthedev reviewed

Jun 22, 2023

View reviewed changes

Comment thread

Doc/whatsnew/3.13.rst Outdated

		:c:expr:`char * const ` in C++ and :c:expr:`char const *` in C
		without explicit type cast.
		This can be overridden by defining the macro :c:macro:`PY_CXX_CONST`
		This can be overridden by defining the macro ``PY_CXX_CONST``

arhadthedev Jun 22, 2023

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Can we use this?

Suggested change

	This can be overridden by defining the macro ``PY_CXX_CONST``
	This can be overridden by defining the macro :c:macro:`!PY_CXX_CONST`

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

serhiy-storchaka Jun 22, 2023

Copy link

Copy Markdown

Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Yes, both :c:macro:`!PY_CXX_CONST` and ``PY_CXX_CONST`` would work.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

CAM-Gerlach Jun 24, 2023

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

To note, while they both silence the warning, they have different semantics in the source and rendering in the output.

However, with the changes suggested above, it is a moot point as you can link this and also simplify this What's New entry by referring readers to the macro description, which explains this (instead of duplicating the explanation here).

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

serhiy-storchaka mentioned this pull request

Jun 22, 2023

PyArg_ParseTupleAndKeywords and fields take (non-const) char *keywords[] capi-workgroup/problems#28

Open

CAM-Gerlach reviewed

Jun 24, 2023

View reviewed changes

Comment thread

Doc/c-api/arg.rst Outdated


		The keywords parameter declaration is :c:expr:`char * const *` in C and
		:c:expr:`const char * const *` in C++.
		This can be overridden by defining the macro :c:macro:`PY_CXX_CONST`

CAM-Gerlach Jun 24, 2023

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

.. macro:: PY_CXX_CONST

   The value to be inserted, if any, before :c:expr:`char * const *`
   in the *keywords* parameter declaration of
   :c:func:`PyArg_ParseTupleAndKeywords`
   and :c:func:`PyArg_VaParseTupleAndKeywords`.
   Default empty for C and ``const`` for C++ (:c:expr:`const char * const *`).
   To override, define it to the desired value before including :file:`Python.h`.

   .. versionadded:: 3.13

You can then simplify this note accordingly, per my other suggestion (in a separate comment).

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

Doc/c-api/arg.rst Outdated

Comment on lines +426 to +428

		This can be overridden by defining the macro :c:macro:`PY_CXX_CONST`
		before including :file:`Python.h` as ``const`` for the latter and as
		empty value for the former.

CAM-Gerlach Jun 24, 2023

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Suggested change

	This can be overridden by defining the macro :c:macro:`PY_CXX_CONST`
	before including :file:`Python.h` as ``const`` for the latter and as
	empty value for the former.
	This can be overridden with the :c:macro:`PY_CXX_CONST` macro.

Refer to the newly-added macro's documentation instead of needing to repeat these details here.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

Doc/whatsnew/3.13.rst Outdated

		:c:expr:`char * const ` in C++ and :c:expr:`char const *` in C
		without explicit type cast.
		This can be overridden by defining the macro :c:macro:`PY_CXX_CONST`
		This can be overridden by defining the macro ``PY_CXX_CONST``

CAM-Gerlach Jun 24, 2023

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

To note, while they both silence the warning, they have different semantics in the source and rendering in the output.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

Doc/whatsnew/3.13.rst Outdated

Comment on lines +436 to +438

		This can be overridden by defining the macro ``PY_CXX_CONST``
		before including :file:`Python.h` as ``const`` for the latter and as
		empty value for the former.

CAM-Gerlach Jun 24, 2023

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Suggested change

	This can be overridden by defining the macro ``PY_CXX_CONST``
	before including :file:`Python.h` as ``const`` for the latter and as
	empty value for the former.
	This can be overridden with the :c:macro:`PY_CXX_CONST` macro.

Now that this is documented, simply refer interested readers here directly instead of duplicating the whole explanation.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

Doc/whatsnew/3.13.rst Outdated

		* The keywords parameter of :c:func:`PyArg_ParseTupleAndKeywords` and
		:c:func:`PyArg_VaParseTupleAndKeywords` has now type :c:expr:`char * const *`
		in C and :c:expr:`const char * const ` in C++, instead of :c:expr:`char *`.
		It makes these functions compatible with argument of type

CAM-Gerlach Jun 24, 2023

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Suggested change

	It makes these functions compatible with argument of type
	It makes these functions compatible with arguments of type

Fix grammar error

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

Doc/whatsnew/3.13.rst Outdated

		It makes these functions compatible with argument of type
		:c:expr:`const char * const `, :c:expr:`const char *` or
		:c:expr:`char * const ` in C++ and :c:expr:`char const *` in C
		without explicit type cast.

CAM-Gerlach Jun 24, 2023

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Suggested change

	without explicit type cast.
	without an explicit type cast.

Fix grammar error

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

CAM-Gerlach commented Jun 24, 2023

Copy link

Copy Markdown

Member

Standard reminder: You can directly apply all the suggestions you want in one go by going to Files changed -> Clicking Add to batch on each suggestion -> When done, clicking Commit.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

serhiy-storchaka and others added 2 commits

December 1, 2023 11:51


          Merge branch 'main' into PyArg_ParseTupleAndKeywords-const

910c5d2


          Apply suggestions.

0e4dc59

Co-authored-by: C.A.M. Gerlach <[email protected]>

serhiy-storchaka force-pushed the PyArg_ParseTupleAndKeywords-const branch from 4ff9e8f to 0e4dc59 Compare

December 1, 2023 10:06

serhiy-storchaka commented Dec 1, 2023

Copy link

Copy Markdown

Member Author

@CAM-Gerlach Thank you, I applied all your suggestions.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

serhiy-storchaka merged commit da6760b into python:main

Dec 4, 2023

bedevere-app Bot removed the awaiting core review label

Dec 4, 2023

serhiy-storchaka deleted the PyArg_ParseTupleAndKeywords-const branch

December 4, 2023 11:15

aisk pushed a commit to aisk/cpython that referenced this pull request

Feb 11, 2024


          pythongh-65210: Add const qualifiers in PyArg_VaParseTupleAndKeywords…

4fb94c8

…() (pythonGH-105958)

Change the declaration of the keywords parameter in functions
PyArg_ParseTupleAndKeywords() and PyArg_VaParseTupleAndKeywords() from `char **`
to `char * const *` in C and `const char * const *` in C++.

It makes these functions compatible with argument of type `const char * const *`,
`const char **` or `char * const *` in C++ and `char * const *` in C
without explicit type cast.

Co-authored-by: C.A.M. Gerlach <[email protected]>

davidhewitt mentioned this pull request

Aug 6, 2024

ffi: update modsupport.rs for Python 3.13 PyO3/pyo3#4420

Merged

Glyphack pushed a commit to Glyphack/cpython that referenced this pull request

Sep 2, 2024


          pythongh-65210: Add const qualifiers in PyArg_VaParseTupleAndKeywords…

a01a009

…() (pythonGH-105958)

Change the declaration of the keywords parameter in functions
PyArg_ParseTupleAndKeywords() and PyArg_VaParseTupleAndKeywords() from `char **`
to `char * const *` in C and `const char * const *` in C++.

It makes these functions compatible with argument of type `const char * const *`,
`const char **` or `char * const *` in C++ and `char * const *` in C
without explicit type cast.

Co-authored-by: C.A.M. Gerlach <[email protected]>

Reviewers

hugovk

hugovk left review comments

arhadthedev

arhadthedev left review comments

CAM-Gerlach

CAM-Gerlach left review comments

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

topic-C-API

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

5 participants

Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.

You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.

Conversation

serhiy-storchaka commented Jun 21, 2023 • edited by github-actions Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

CAM-Gerlach commented Jun 24, 2023

Uh oh!

serhiy-storchaka commented Dec 1, 2023

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

serhiy-storchaka commented Jun 21, 2023 •

edited by github-actions Bot

Loading

Uh oh!

There was an error while loading. Please reload this page.