-
-
Notifications
You must be signed in to change notification settings - Fork 30.9k
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-50333: Deprecate support of non-tuple sequences in PyArg_ParseTuple() #128374
base: main
Are you sure you want to change the base?
gh-50333: Deprecate support of non-tuple sequences in PyArg_ParseTuple() #128374
Conversation
…seTuple() Non-tuple sequences are deprecated as argument for the "(items)" format unit in PyArg_ParseTuple() and other argument parsing functions if items contains format units which store borrowed buffer or reference (e.g. "s" and "O"). str and bytearray are no longer accepted as valid sequences.
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.
While I understand that "borrowed buffer or reference" reads as "borrowed buffer or borrowed reference", I would advise repeating "borrowed reference" as well.
I haven't looked at te implementation though.
Misc/NEWS.d/next/C_API/2024-12-31-15-28-14.gh-issue-50333.KxQUXa.rst
Outdated
Show resolved
Hide resolved
Co-authored-by: Bénédikt Tran <[email protected]> Co-authored-by: Stan Ulbrych <[email protected]>
In this case, I think we should consider being explicit, rather than worrying about the repeated word. |
levels[0] = 0; | ||
PyOS_snprintf(msgbuf, bufsize, | ||
"must be %d-item sequence, not %.50s", | ||
"must be %d-item tuple, not %.50s", |
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.
snprintf
will keep the string within bufsize
; we should not need to use the sized specifier.
"must be %d-item tuple, not %.50s", | |
"must be %d-item tuple, not %s", |
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.
This is for consistency with other formats. Also, if we will add more text after the type name, it is easier to not forget to truncate.
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.
No worry; snprintf
will truncate.
if (PyTuple_Check(arg)) { | ||
Py_INCREF(arg); | ||
} |
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.
We can easily avoid the unneeded incref/decref if we refactor out the convertitem
loop. I'll make a suggestion to your fork for your consideration.
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.
Suggestion opened:
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 thought about this, but I am not sure that there is a significant benefit in this. It would complicate the code, for sure. So I left the simpler code.
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.
So I left the simpler code.
However, the "simpler" code is more complex with regards to reference counting. IMO, keeping the ref counting simple is worth it; ref count bugs are hard to catch. But it is your call. BTW, I addressed your remarks on your fork.
Co-authored-by: Erlend E. Aasland <[email protected]>
Non-tuple sequences are deprecated as argument for the "(items)" format unit in PyArg_ParseTuple() and other argument parsing functions if items contains format units which store borrowed buffer or reference (e.g. "s" and "O").
str and bytearray are no longer accepted as valid sequences.
📚 Documentation preview 📚: https://cpython-previews--128374.org.readthedocs.build/