Skip to content

bpo-40130: _PyUnicode_AsKind() should not be exported.#19265

Merged
serhiy-storchaka merged 2 commits into
python:masterfrom
serhiy-storchaka:pyunicode_askind
Apr 1, 2020
Merged

bpo-40130: _PyUnicode_AsKind() should not be exported.#19265
serhiy-storchaka merged 2 commits into
python:masterfrom
serhiy-storchaka:pyunicode_askind

Conversation

@serhiy-storchaka

@serhiy-storchaka serhiy-storchaka commented Mar 31, 2020

Copy link
Copy Markdown
Member

Make it a static function, and pass known attributes (kind, data, length) instead of the PyUnicode object.

https://bugs.python.org/issue40130

Make it a static function, and pass known attributes
(kind, data, length) instead of the PyUnicode object.

@vstinner vstinner left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM. It's fine to remove private functions whenever we want, especially before a release.

First, I wasn't sure if it's a good idea to require the (kind, data, len) triplet rather than PyObject*, but after I read the issue, I think that it's ok.

Comment thread Objects/unicodeobject.c
return;
}
else {
else if (kind == PyUnicode_4BYTE_KIND) {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Maybe a switch/case would be more appropriate.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Maybe. I left a chain of ifs because I am sure that in this case the compiler would generate the optimal code for PyUnicode_1BYTE_KIND, and I am not so sure about this in the case of switch/case. It may be a matter of other issue, I do not want to introduce regression.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

It was just a remark. You can leave the code as it is, I already approved your PR.

Comment thread Objects/unicodeobject.c Outdated
Py_UCS1, Py_UCS2,
PyUnicode_1BYTE_DATA(s),
PyUnicode_1BYTE_DATA(s) + len,
(Py_UCS1 *)data,

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

it should be const Py_UCS1*, no?

@serhiy-storchaka serhiy-storchaka merged commit 17b4733 into python:master Apr 1, 2020
@serhiy-storchaka serhiy-storchaka deleted the pyunicode_askind branch April 1, 2020 12:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

skip news type-feature A feature request or enhancement

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants