[issue16281] TODO in tailmatch(): it does not support backward in all cases

[STINNER Victor]

STINNER Victor added the comment:

> Shouldn't this be applied to 3.3?

It's just a cleanup, it doesn't fix any real bug. I prefer to not
pollute old versions with cleanup.

> As for optimization, I made some benchmarks and didn't saw any significant 
> difference. Usually this function used to check short ASCII heads and tails 
> and any optimization will not be seen even under a microscope.

Ok, agreed.

--

___
Python tracker 

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

[issue16281] TODO in tailmatch(): it does not support backward in all cases

[Serhiy Storchaka]

Serhiy Storchaka added the comment:

Shouldn't this be applied to 3.3?

As for optimization, I made some benchmarks and didn't saw any significant 
difference. Usually this function used to check short ASCII heads and tails and 
any optimization will not be seen even under a microscope.

--

___
Python tracker 

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

[issue16281] TODO in tailmatch(): it does not support backward in all cases

[STINNER Victor]

STINNER Victor added the comment:

"Here I see a one obvious opportunity for optimization: ..."

@Serhiy: Can you please open a new issue for this? I consider the issue as 
fixed: I just removed the TODO (for the reason explained in the changeset).

--

___
Python tracker 

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

[issue16281] TODO in tailmatch(): it does not support backward in all cases

[Roundup Robot]

Roundup Robot added the comment:

New changeset 49eb2488145d by Victor Stinner in branch 'default':
Close #16281: handle tailmatch() failure and remove useless comment
http://hg.python.org/cpython/rev/49eb2488145d

--
nosy: +python-dev
resolution:  -> fixed
stage: needs patch -> committed/rejected
status: open -> closed

___
Python tracker 

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

[issue16281] TODO in tailmatch(): it does not support backward in all cases

[Serhiy Storchaka]

Changes by Serhiy Storchaka :


--
components: +Interpreter Core
stage:  -> needs patch

___
Python tracker 

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

[issue16281] TODO in tailmatch(): it does not support backward in all cases

[STINNER Victor]

STINNER Victor added the comment:

>> Oh, PyUnicode_Tailmatch() documentation doesn't mention that the function
>> can fail.
>
> But it does.
>
> .. c:function:: int PyUnicode_Tailmatch(PyObject *str, PyObject *substr, \
> Py_ssize_t start, Py_ssize_t end, int direction)
>
>Return 1 if *substr* matches ``str[start:end]`` at the given tail end
>(*direction* == -1 means to do a prefix match, *direction* == 1 a suffix 
> match),
>0 otherwise. Return ``-1`` if an error occurred.

Oh, I read the "documentation" in unicodeobject.h:

/* Return 1 if substr matches str[start:end] at the given tail end, 0
   otherwise. */

The problem is that tailmatch() returns 0 if PyUnicode_READY() failed.
It is a bug, even if it cannot occur because PyUnicode_Tailmatch()
checks that the both strings are ready.

--

___
Python tracker 

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

[issue16281] TODO in tailmatch(): it does not support backward in all cases

[Serhiy Storchaka]

Serhiy Storchaka added the comment:

> Oh, PyUnicode_Tailmatch() documentation doesn't mention that the function
> can fail.

But it does.

.. c:function:: int PyUnicode_Tailmatch(PyObject *str, PyObject *substr, \
Py_ssize_t start, Py_ssize_t end, int direction)

   Return 1 if *substr* matches ``str[start:end]`` at the given tail end
   (*direction* == -1 means to do a prefix match, *direction* == 1 a suffix 
match),
   0 otherwise. Return ``-1`` if an error occurred.

--

___
Python tracker 

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

[issue16281] TODO in tailmatch(): it does not support backward in all cases

[STINNER Victor]

STINNER Victor added the comment:

Oh, PyUnicode_Tailmatch() documentation doesn't mention that the function can 
fail.

--

___
Python tracker 

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

[issue16281] TODO in tailmatch(): it does not support backward in all cases

[Serhiy Storchaka]

Serhiy Storchaka added the comment:

The result does not depend on the direction of comparison. This only affects 
speed. But who can to say in which direction comparison will be faster?

Here I see a one obvious opportunity for optimization:

if (kind_self < kind_sub)
return 0;

After that and after processing the case (kind_self == kind_sub) only 3 special 
cases left: UCS1 in UCS2, UCS1 in UCS4, and UCS2 in UCS4.  Get rid of slow 
PyUnicode_READ() for this cases will speed up the code.  Also note that 
comparing first and last characters before memcmp can be a slowdown (because 
PyUnicode_READ() is slow).  Try to compare first and last bytes.

--

___
Python tracker 

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

[issue16281] TODO in tailmatch(): it does not support backward in all cases

[STINNER Victor]

New submission from STINNER Victor:

Oh oh, it looks like the implementation of tailmatch() was not finished:

/* If both are of the same kind, memcmp is sufficient */
if (kind_self == kind_sub) {
return ...;
}
/* otherwise we have to compare each character by first accesing it */
else {
/* We do not need to compare 0 and len(substring)-1 because
   the if statement above ensured already that they are equal
   when we end up here. */
/* TODO: honor direction and do a forward or backwards search */
for (i = 1; i < end_sub; ++i) {
if (PyUnicode_READ(kind_self, data_self, offset + i) !=
PyUnicode_READ(kind_sub, data_sub, i))
return 0;
}
return 1;
}

--
messages: 173301
nosy: haypo, loewis, serhiy.storchaka
priority: normal
severity: normal
status: open
title: TODO in tailmatch(): it does not support backward in all cases
versions: Python 3.3, Python 3.4

___
Python tracker 

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