[issue28701] Replace PyUnicode_CompareWithASCIIString with _PyUnicode_EqualToASCIIString

[Roundup Robot]

Roundup Robot added the comment:

New changeset 35334a4d41aa by Stefan Krah in branch '3.5':
Issue #28701: Revert part of 5bdc8e1a50c8 for the following reasons:
https://hg.python.org/cpython/rev/35334a4d41aa

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue28701] Replace PyUnicode_CompareWithASCIIString with _PyUnicode_EqualToASCIIString

[Stefan Krah]

Stefan Krah added the comment:

For the record: This is all that happened in decimal if a) you
pass a legacy string and b) force _PyUnicode_Ready() to throw
a MemoryError:

>>> from decimal import *
>>> import _testcapi
>>> context = Context()
>>> traps = _testcapi.unicode_legacy_string('traps')
>>> getattr(context, traps)
Traceback (most recent call last):
  File "", line 1, in 
AttributeError



Both a) and b) are not trivial to accomplish at all and the result
is completely benign.

--
nosy: +skrah

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue28701] Replace PyUnicode_CompareWithASCIIString with _PyUnicode_EqualToASCIIString

[Serhiy Storchaka]

Changes by Serhiy Storchaka :


--
resolution:  -> fixed
status: open -> closed

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue28701] Replace PyUnicode_CompareWithASCIIString with _PyUnicode_EqualToASCIIString

[Roundup Robot]

Roundup Robot added the comment:

New changeset 6dd22ed7140e by Serhiy Storchaka in branch '3.6':
Issue #28701: _PyUnicode_EqualToASCIIId and _PyUnicode_EqualToASCIIString now
https://hg.python.org/cpython/rev/6dd22ed7140e

New changeset 44874b20e612 by Serhiy Storchaka in branch 'default':
Issue #28701: _PyUnicode_EqualToASCIIId and _PyUnicode_EqualToASCIIString now
https://hg.python.org/cpython/rev/44874b20e612

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue28701] Replace PyUnicode_CompareWithASCIIString with _PyUnicode_EqualToASCIIString

[STINNER Victor]

STINNER Victor added the comment:

_PyUnicode_EqualToASCII-runtime-check.diff LGTM.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue28701] Replace PyUnicode_CompareWithASCIIString with _PyUnicode_EqualToASCIIString

[Serhiy Storchaka]

Serhiy Storchaka added the comment:

Following patch adds checks in debug mode that the right argument of 
_PyUnicode_EqualToASCIIString and _PyUnicode_EqualToASCIIId is ASCII-only 
string.

--
Added file: 
http://bugs.python.org/file45505/_PyUnicode_EqualToASCII-runtime-check.diff

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue28701] Replace PyUnicode_CompareWithASCIIString with _PyUnicode_EqualToASCIIString

[Roundup Robot]

Roundup Robot added the comment:

New changeset b607f835f170 by Serhiy Storchaka in branch '3.5':
Fixed an off-by-one error in _PyUnicode_EqualToASCIIString (issue #28701).
https://hg.python.org/cpython/rev/b607f835f170

New changeset 1369e51182b7 by Serhiy Storchaka in branch '3.6':
Fixed an off-by-one error in _PyUnicode_EqualToASCIIString (issue #28701).
https://hg.python.org/cpython/rev/1369e51182b7

New changeset ba14f8b61bd8 by Serhiy Storchaka in branch 'default':
Fixed an off-by-one error in _PyUnicode_EqualToASCIIString (issue #28701).
https://hg.python.org/cpython/rev/ba14f8b61bd8

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue28701] Replace PyUnicode_CompareWithASCIIString with _PyUnicode_EqualToASCIIString

[Serhiy Storchaka]

Serhiy Storchaka added the comment:

The correct issue for above commits is issue21449.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue28701] Replace PyUnicode_CompareWithASCIIString with _PyUnicode_EqualToASCIIString

[Roundup Robot]

Roundup Robot added the comment:

New changeset faf04a995031 by Serhiy Storchaka in branch '3.5':
Issue #28701: Replace _PyUnicode_CompareWithId with _PyUnicode_EqualToASCIIId.
https://hg.python.org/cpython/rev/faf04a995031

New changeset ff3dacc98b3a by Serhiy Storchaka in branch '3.6':
Issue #28701: Replace _PyUnicode_CompareWithId with _PyUnicode_EqualToASCIIId.
https://hg.python.org/cpython/rev/ff3dacc98b3a

New changeset 765013f71bc4 by Serhiy Storchaka in branch 'default':
Issue #28701: Replace _PyUnicode_CompareWithId with _PyUnicode_EqualToASCIIId.
https://hg.python.org/cpython/rev/765013f71bc4

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue28701] Replace PyUnicode_CompareWithASCIIString with _PyUnicode_EqualToASCIIString

[STINNER Victor]

STINNER Victor added the comment:

I suggest "return 0 in release build and crash in debug build".

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue28701] Replace PyUnicode_CompareWithASCIIString with _PyUnicode_EqualToASCIIString

[Serhiy Storchaka]

Serhiy Storchaka added the comment:

> Can you please also document the behaviour if you pass two non-ASCII strings 
> which are equal?

What mean "equal"? The left argument is a Unicode string, but the right 
argument is a byte string. For comparing them we should decode right argument 
or encode left argument. The result depends on using encoding. 
_PyUnicode_EqualToASCIIString() uses ASCII (as shown from its name). Non-ASCII 
strings can't be equal. This is documented.

If the documentation is not clear, could you provide better wording?

> Maybe the API should be more strict and require right to be ASCII: "right 
> string must be encoded to ASCII". I expect an assertion error or a fatal 
> error if right is non-ASCII when Python is compiled in debug mode.

I hesitated about adding an assertion error or a fatal error in a bug fix. But 
this can be added in develop version.

I don't know what is better -- return 0 in all builds or return 0 in release 
build and crash in debug build?

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue28701] Replace PyUnicode_CompareWithASCIIString with _PyUnicode_EqualToASCIIString

[STINNER Victor]

STINNER Victor added the comment:

(I reopen the issue to ask my question :-))

+/* Test whether a unicode is equal to ASCII string.  Return 1 if true,
+   0 otherwise.  Return 0 if any argument contains non-ASCII characters.
+   Any error occurs inside will be cleared before return. */

Can you please also document the behaviour if you pass two non-ASCII strings 
which are equal? I understand that it returns also 0, right?

Maybe the API should be more strict and require right to be ASCII: "right 
string must be encoded to ASCII". I expect an assertion error or a fatal error 
if right is non-ASCII when Python is compiled in debug mode.

--
resolution: fixed -> 
status: closed -> open

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue28701] Replace PyUnicode_CompareWithASCIIString with _PyUnicode_EqualToASCIIString

[Serhiy Storchaka]

Serhiy Storchaka added the comment:

Thanks Xiang and Inada for your reviews.

The patch fixes a bug: error is not checked after 
PyUnicode_CompareWithASCIIString(). The patch is not applicable to 2.7 since 
PyUnicode_CompareWithASCIIString() is Python 3 only.

--
assignee:  -> serhiy.storchaka
resolution:  -> fixed
stage: patch review -> resolved
status: open -> closed
versions:  -Python 2.7

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue28701] Replace PyUnicode_CompareWithASCIIString with _PyUnicode_EqualToASCIIString

[Roundup Robot]

Roundup Robot added the comment:

New changeset 386c682dcd75 by Serhiy Storchaka in branch '3.5':
Issue #28701: Replace PyUnicode_CompareWithASCIIString with 
_PyUnicode_EqualToASCIIString.
https://hg.python.org/cpython/rev/386c682dcd75

New changeset 72d07d13869a by Serhiy Storchaka in branch '3.6':
Issue #28701: Replace PyUnicode_CompareWithASCIIString with 
_PyUnicode_EqualToASCIIString.
https://hg.python.org/cpython/rev/72d07d13869a

New changeset 6f0f77333da5 by Serhiy Storchaka in branch 'default':
Issue #28701: Replace PyUnicode_CompareWithASCIIString with 
_PyUnicode_EqualToASCIIString.
https://hg.python.org/cpython/rev/6f0f77333da5

--
nosy: +python-dev

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue28701] Replace PyUnicode_CompareWithASCIIString with _PyUnicode_EqualToASCIIString

[INADA Naoki]

INADA Naoki added the comment:

Patch LGTM.
But I don't know it's OK to commit it on 2.7, 3.5 and 3.6.

--
nosy: +inada.naoki

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue28701] Replace PyUnicode_CompareWithASCIIString with _PyUnicode_EqualToASCIIString

[Xiang Zhang]

Xiang Zhang added the comment:

LGTM.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue28701] Replace PyUnicode_CompareWithASCIIString with _PyUnicode_EqualToASCIIString

[Serhiy Storchaka]

Changes by Serhiy Storchaka :


Added file: 
http://bugs.python.org/file45491/PyUnicode_CompareWithASCIIString.cocci

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue28701] Replace PyUnicode_CompareWithASCIIString with _PyUnicode_EqualToASCIIString

[Serhiy Storchaka]

New submission from Serhiy Storchaka:

Proposed patch replaces calls of public function 
PyUnicode_CompareWithASCIIString() with new private function 
_PyUnicode_EqualToASCIIString(). The problem with  
PyUnicode_CompareWithASCIIString() is that it returns -1 for the result "less 
than" and error, but the error case is never checked. The patch is purposed for 
following purposes:

1. Readability. ``_PyUnicode_EqualToASCIIString(...)`` looks more readable than 
``PyUnicode_CompareWithASCIIString(...) == 0`` or 
``!PyUnicode_CompareWithASCIIString(...)``, especially in large expression. I 
always have to make an effort to understand correctly the meaning of the latter 
expression.

2. Efficiency. If the strings are not equal, _PyUnicode_EqualToASCIIString() 
can quickly return false, but PyUnicode_CompareWithASCIIString() needs to check 
whether what string is larger.

3. Correctness. Since no caller checks the error of 
PyUnicode_CompareWithASCIIString(), it is incorrectly interpreted as "less 
then". Exception set by PyUnicode_CompareWithASCIIString() can be leaked in 
following code causing mystical error or crash in debug build. There are too 
many callers to add error checking for them all. These would be non-trivial 
error-prone changes that add new lines of the code, new variables and new 
returns or gotos. On other hand replacing PyUnicode_CompareWithASCIIString() 
with _PyUnicode_EqualToASCIIString() is done by simple script.

_PyUnicode_EqualToASCIIString() returns true value (1) if strings are equal, 
false value (0) if they are different, and doesn't raise exceptions. Unlike to 
PyUnicode_CompareWithASCIIString() it works only with ASCII characters and 
returns false if any string contains non-ASCII characters.

The patch also documents the return value of PyUnicode_CompareWithASCIIString() 
in case of error.

See issue21449 for similar issue with _PyUnicode_CompareWithId().

--
components: Interpreter Core, Unicode
files: _PyUnicode_EqualToASCIIString.patch
keywords: patch
messages: 280882
nosy: ezio.melotti, haypo, josh.r, serhiy.storchaka, xiang.zhang
priority: normal
severity: normal
stage: patch review
status: open
title: Replace PyUnicode_CompareWithASCIIString with 
_PyUnicode_EqualToASCIIString
type: behavior
versions: Python 2.7, Python 3.5, Python 3.6, Python 3.7
Added file: http://bugs.python.org/file45490/_PyUnicode_EqualToASCIIString.patch

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com