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

Jump to bottom

Minor: fix unicode-related inline function warnings #93011

Merged
merged 1 commit into from May 23, 2022
Merged

Minor: fix unicode-related inline function warnings #93011

merged 1 commit into from May 23, 2022

Conversation

wjakob
Copy link
Contributor

@wjakob wjakob commented May 20, 2022

When compiling my extension module against latest main version of CPython, I get many warnings related to unused parameters in unicodeobject.h. Example:

In file included from /Users/wjakob/nanobind/src/tensor.cpp:1:
In file included from /Users/wjakob/nanobind/include/nanobind/tensor.h:14:
In file included from /Users/wjakob/nanobind/include/nanobind/nanobind.h:37:
In file included from /Users/wjakob/nanobind/include/nanobind/nb_python.h:22:
In file included from /Users/wjakob/cpython-dist/include/python3.12/Python.h:51:
In file included from /Users/wjakob/cpython-dist/include/python3.12/unicodeobject.h:1029:
/Users/wjakob/cpython-dist/include/python3.12/cpython/unicodeobject.h:196:57: warning: unused parameter 'op' [-Wunused-parameter]
static inline unsigned int PyUnicode_IS_READY(PyObject *op) {
                                                        ^
/Users/wjakob/cpython-dist/include/python3.12/cpython/unicodeobject.h:416:45: warning: unused parameter 'op' [-Wunused-parameter]
static inline int PyUnicode_READY(PyObject *op)
                                            ^

This PR fixes those warnings. This isn't really a bug in Python, so I did not create a corresponding issue tracker ticket.

@cpython-cla-bot
Copy link

@cpython-cla-bot cpython-cla-bot bot commented May 20, 2022 •
edited

All commit authors signed the Contributor License Agreement.
CLA signed

@bedevere-bot
Copy link

@bedevere-bot bedevere-bot commented May 20, 2022

Most changes to Python require a NEWS entry.

Please add it using the blurb_it web app or the blurb command-line tool.

@wjakob
Copy link
Contributor Author

@wjakob wjakob commented May 20, 2022 •
edited

@bedevere-bot (This issue seems too minor to warrant an issue number or a NEWS entry)

JelleZijlstra
JelleZijlstra approved these changes May 22, 2022
View changes
Copy link
Member

@JelleZijlstra JelleZijlstra left a comment

Thanks for your contribution! cc @vstinner who worked on this file recently.

I think we can skip the news entry here because the problem it fixes hasn't been in any release (it's in the main branch only). If it had been in a release, we should have added a NEWS entry because it's a user-visible change.

I'll leave this open for a few days in case Victor or others have comments, and then merge.

@JelleZijlstra JelleZijlstra self-assigned this May 22, 2022
@vstinner
Copy link
Member

@vstinner vstinner commented May 23, 2022

Thanks for your contribution! cc @vstinner who worked on this file recently.

cc @methane who introduced the warning.

vstinner
vstinner reviewed May 23, 2022
View changes
Include/cpython/unicodeobject.h Outdated Show resolved Hide resolved
Include/cpython/unicodeobject.h Outdated Show resolved Hide resolved
@wjakob
Copy link
Contributor Author

@wjakob wjakob commented May 23, 2022

(Commit amended with tweaked syntax)

vstinner
vstinner approved these changes May 23, 2022
View changes
Copy link
Member

@vstinner vstinner left a comment

LGTM.

@vstinner vstinner merged commit b2694ab into python:main May 23, 2022
12 checks passed
@vstinner
Copy link
Member

@vstinner vstinner commented May 23, 2022

Merged, thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vstinner vstinner

@methane methane

@JelleZijlstra JelleZijlstra

Assignees

@JelleZijlstra JelleZijlstra

Labels
skip issue skip news
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@wjakob @bedevere-bot @vstinner @methane @JelleZijlstra