-
-
Notifications
You must be signed in to change notification settings - Fork 31.4k
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
Cut disused recode_encoding logic in _PyBytes_DecodeEscape. #16013
Conversation
All call sites pass NULL for `recode_encoding`, so this path is completely untested. That's been true since before Python 3.0. It adds significant complexity to this logic, so it's best to take it out. All call sites now have a literal NULL, and that's been true since commit 768921c eliminated a conditional (`foo ? bar : NULL`) at the call site in Python/ast.c where we're parsing a bytes literal. But even before then, that condition `foo` had been a constant since unadorned string literals started meaning Unicode, in commit 572dbf8 aka v3.0a1~1035 . The `unicode` parameter is already unused, so mark it as unused too. The code that acted on it was also taken out before Python 3.0, in commit 8d30cc0 aka v3.0a1~1031 . The function (PyBytes_DecodeEscape) is exposed in the API, but it's never been documented.
Is there a reason not to eliminate the |
agreed, this is a static internal only function. just remove the unused parameters. |
|
Thanks both for the reviews!
That sounds even better 🙂 The reason I hesitated is that these do appear in
and in particular I wasn't sure changing the signatures would be acceptable, so I went for the most minimal change. I'll update this PR to drop these parameters from What are the criteria for saying an API is internal and can be freely changed? I see this one has two internal-flavored markings: the leading |
As described in https://docs.python.org/3/c-api/intro.html#include-files, no extensions should use |
python/cpython#16013 python/cpython@3a4f667 removed two arguments from _PyBytes_DecodeEscape . This calls _PyBytes_DecodeEscape with the new signature if the minor version is 9 or greater.
Well, it was also called from typed_ast/ast3/Python/ast.c (which is an external backport of ast.c and uses some internals like this one), but we're fixing that now, so no need to roll this back or anything. See python/typed_ast#128 |
This PR and commit to cpython: python/cpython#16013 python/cpython@3a4f667 removed two arguments from _PyBytes_DecodeEscape . We now call _PyBytes_DecodeEscape with the new signature if the (CPython) minor version is 9 or greater.
All call sites pass NULL for
recode_encoding
, so this path iscompletely untested. That's been true since before Python 3.0.
It adds significant complexity to this logic, so it's best to
take it out.
All call sites now have a literal NULL, and that's been true since
commit 768921c eliminated a conditional (
foo ? bar : NULL
) atthe call site in Python/ast.c where we're parsing a bytes literal.
But even before then, that condition
foo
had been a constantsince unadorned string literals started meaning Unicode, in commit
572dbf8 aka v3.0a1~1035 .
The
unicode
parameter is already unused, so mark it as unused too.The code that acted on it was also taken out before Python 3.0, in
commit 8d30cc0 aka v3.0a1~1031 .
The function (PyBytes_DecodeEscape) is exposed in the API, but it's
never been documented.