classification
Title: Factor out common bytes and bytearray implementation
Type: enhancement Stage: resolved
Components: Interpreter Core Versions: Python 3.6
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: Nosy List: meador.inge, python-dev, serhiy.storchaka, terry.reedy, vstinner
Priority: normal Keywords: patch

Created on 2016-04-15 08:05 by serhiy.storchaka, last changed 2017-10-22 07:34 by serhiy.storchaka. This issue is now closed.

Files
File name Uploaded Description Edit
bytes_methods.patch serhiy.storchaka, 2016-04-15 08:05 review
bytes_methods_stage1.patch serhiy.storchaka, 2016-04-18 06:52 review
bytes_methods_stage2.patch serhiy.storchaka, 2016-05-04 20:11 review
bytes_methods_stage3.patch serhiy.storchaka, 2016-05-05 06:38 review
bytes_methods_stage4.patch serhiy.storchaka, 2016-05-05 06:57 review
Messages (16)
msg263458 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2016-04-15 08:05
Proposed patch factors out the implementation of bytes and bytearray by moving common code in separate files. This is not new approach, the part of the code is already shared.

The patch decreases the size of the source code by 862 lines.
msg263630 - (view) Author: Meador Inge (meador.inge) * (Python committer) Date: 2016-04-17 21:13
If I follow this correctly, then it seems like there are two
main pieces to this patch:

  1. Consolidating the following methods into `bytes_methods.c`:

     a. {bytes, bytearray}_find_internal

     b. {bytes, bytearray}_find

     c. {bytes, bytearray}_count

     d. {bytes, bytearray}_index

     e. {bytes, bytearray}_rfind

     f. {bytes, bytearray}_rindex

     g. {bytes, bytearray}_contains

     h. (_bytes, _bytearray}_tailmatch

     i. {bytes, bytearray}_startswith

     j. {bytes, bytearray}_endswith

  2. Consolidating the redundant implementations of the following functions
     into the stringlib:

     a. return_self

     b. countchar

     c. replace_interleave

     d. replace_delete_single_character

     e. replace_delete_substring

     f. replace_single_character_in_place

     g. replace_substring_in_place

     h. replace_single_character

     i. replace_substring

     j. replace

If so, then that seems reasonable to me and a nice cleanup.

A few comments:

  1. The diffs are fairly large.  It might be easier to follow if you stage
     the two steps above as separate commits (if possible).

  2. Why are some of the namings in stringlib inconsistent?  For example,
     `stringlib_method_find` and `stringlib_index`.  In other words, why
     do some names have the *_method_* part?
msg263647 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2016-04-18 06:52
>   1. The diffs are fairly large.  It might be easier to follow if you stage
>      the two steps above as separate commits (if possible).

OK. Here is the first part of the patch. It moves common code and docstrings in bytes_methods.c.

>   2. Why are some of the namings in stringlib inconsistent?  For example,
>      `stringlib_method_find` and `stringlib_index`.  In other words, why
>      do some names have the *_method_* part?

There were conflicts with existing names stringlib_find and stringlib_rfind.
msg264850 - (view) Author: Roundup Robot (python-dev) (Python triager) Date: 2016-05-04 19:30
New changeset 41969033eb9d by Serhiy Storchaka in branch 'default':
Issue #26765: Moved common code and docstrings for bytes and bytearray methods
https://hg.python.org/cpython/rev/41969033eb9d
msg264857 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2016-05-04 20:11
Here is the second part of the patch. It moves common code for replace() method to template file Objects/stringlib/transmogrify.h.
msg264872 - (view) Author: Meador Inge (meador.inge) * (Python committer) Date: 2016-05-05 01:54
LGTM.
msg264878 - (view) Author: Roundup Robot (python-dev) (Python triager) Date: 2016-05-05 06:26
New changeset f4406d746d27 by Serhiy Storchaka in branch 'default':
Issue #26765: Moved common code for the replace() method of bytes and bytearray
https://hg.python.org/cpython/rev/f4406d746d27
msg264879 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2016-05-05 06:38
Thank you Meador.

Stage 3 patch is simple. It just adds guards in bytes-specific and unicode-specific template files to be sure that they are not used with wrong type.
msg264881 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2016-05-05 06:57
Stage 4 patch moves thin wrappers around functions from bytes_methods.c to Objects/stringlib/transmogrify.h. The _method_ suffix is added to some names to distinguish them from existing stringlib names.

I'm not sure this is a good place. It may be better to add separate file (I'm going to move more simple wrappers here), but I don't have good name for it (bytes_methods.h is already used). Any ideas?
msg265672 - (view) Author: Roundup Robot (python-dev) (Python triager) Date: 2016-05-16 06:42
New changeset c6c1882ecc77 by Serhiy Storchaka in branch 'default':
Issue #26765: Ensure that bytes- and unicode-specific stringlib files are used
https://hg.python.org/cpython/rev/c6c1882ecc77
msg269674 - (view) Author: Roundup Robot (python-dev) (Python triager) Date: 2016-07-01 14:57
New changeset b0087e17cd5e by Serhiy Storchaka in branch 'default':
Issue #26765: Moved wrappers for bytes and bytearray methods to common header
https://hg.python.org/cpython/rev/b0087e17cd5e
msg269744 - (view) Author: Terry J. Reedy (terry.reedy) * (Python committer) Date: 2016-07-02 23:19
Serhiy, compiling 3.6 on Windows after this patch lets it start, but causes it to crash both importing tkinter and running patchcheck.  Compiling 3.6 after your previous patch for #27007 and the test suite runs without error.
msg269747 - (view) Author: Terry J. Reedy (terry.reedy) * (Python committer) Date: 2016-07-03 00:07
Checking, I see that 3 of 4 Windows buildbots ran with this patch, so there must be something significantly different about my VS install with respect to this particular patch.  #26624 might or might not have something to do with it (see my posts today).  I can't tell from the logs which update the buildbots are running.

The fourth, x86windows7 crashed before running anything (http://buildbot.python.org/all/builders/x86%20Windows7%203.x/builds/11283/steps/test/logs/stdio), after having previously run part way through the tests.  It *might* have a similar problem, or might not.
msg269758 - (view) Author: Roundup Robot (python-dev) (Python triager) Date: 2016-07-03 10:27
New changeset 0638047c0c36 by Serhiy Storchaka in branch 'default':
Issue #26765: Fixed parsing Py_ssize_t arguments on 32-bit Windows.
https://hg.python.org/cpython/rev/0638047c0c36
msg269759 - (view) Author: Roundup Robot (python-dev) (Python triager) Date: 2016-07-03 10:58
New changeset 6c4c0a23fabb by Serhiy Storchaka in branch 'default':
Backed out changeset b0087e17cd5e (issue #26765)
https://hg.python.org/cpython/rev/6c4c0a23fabb
msg269760 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2016-07-03 10:59
I don't know what is wrong with this patch. Backed out just for the case.
History
Date User Action Args
2017-10-22 07:34:48serhiy.storchakasetstatus: open -> closed
resolution: fixed
stage: patch review -> resolved
2016-07-03 10:59:33serhiy.storchakasetstatus: closed -> open
resolution: fixed -> (no value)
messages: + msg269760

stage: resolved -> patch review
2016-07-03 10:58:11python-devsetmessages: + msg269759
2016-07-03 10:27:50python-devsetmessages: + msg269758
2016-07-03 00:07:39terry.reedysetmessages: + msg269747
2016-07-02 23:19:24terry.reedysetnosy: + terry.reedy
messages: + msg269744
2016-07-02 20:03:12serhiy.storchakasetstatus: open -> closed
resolution: fixed
stage: patch review -> resolved
2016-07-01 14:57:50python-devsetmessages: + msg269674
2016-05-16 06:42:59python-devsetmessages: + msg265672
2016-05-12 14:06:05serhiy.storchakasetnosy: + vstinner
2016-05-05 06:57:36serhiy.storchakasetfiles: + bytes_methods_stage4.patch

messages: + msg264881
2016-05-05 06:38:24serhiy.storchakasetfiles: + bytes_methods_stage3.patch

messages: + msg264879
2016-05-05 06:26:30python-devsetmessages: + msg264878
2016-05-05 01:54:13meador.ingesetmessages: + msg264872
2016-05-04 20:11:45serhiy.storchakasetfiles: + bytes_methods_stage2.patch
2016-05-04 20:11:22serhiy.storchakasetmessages: + msg264857
2016-05-04 19:30:07python-devsetnosy: + python-dev
messages: + msg264850
2016-04-18 06:52:32serhiy.storchakasetfiles: + bytes_methods_stage1.patch

messages: + msg263647
2016-04-17 21:13:45meador.ingesetnosy: + meador.inge

messages: + msg263630
stage: patch review
2016-04-15 08:05:59serhiy.storchakacreate