Skip to content
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

Factor out common bytes and bytearray implementation #70952

Closed
serhiy-storchaka opened this issue Apr 15, 2016 · 16 comments
Closed

Factor out common bytes and bytearray implementation #70952

serhiy-storchaka opened this issue Apr 15, 2016 · 16 comments
Labels
interpreter-core (Objects, Python, Grammar, and Parser dirs) type-feature A feature request or enhancement

Comments

@serhiy-storchaka
Copy link
Member

BPO 26765
Nosy @terryjreedy, @vstinner, @meadori, @serhiy-storchaka
Files
  • bytes_methods.patch
  • bytes_methods_stage1.patch
  • bytes_methods_stage2.patch
  • bytes_methods_stage3.patch
  • bytes_methods_stage4.patch
  • Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.

    Show more details

    GitHub fields:

    assignee = None
    closed_at = <Date 2017-10-22.07:34:48.949>
    created_at = <Date 2016-04-15.08:05:59.101>
    labels = ['interpreter-core', 'type-feature']
    title = 'Factor out common bytes and bytearray implementation'
    updated_at = <Date 2017-10-22.07:34:48.949>
    user = 'https://github.com/serhiy-storchaka'

    bugs.python.org fields:

    activity = <Date 2017-10-22.07:34:48.949>
    actor = 'serhiy.storchaka'
    assignee = 'none'
    closed = True
    closed_date = <Date 2017-10-22.07:34:48.949>
    closer = 'serhiy.storchaka'
    components = ['Interpreter Core']
    creation = <Date 2016-04-15.08:05:59.101>
    creator = 'serhiy.storchaka'
    dependencies = []
    files = ['42465', '42502', '42724', '42729', '42730']
    hgrepos = []
    issue_num = 26765
    keywords = ['patch']
    message_count = 16.0
    messages = ['263458', '263630', '263647', '264850', '264857', '264872', '264878', '264879', '264881', '265672', '269674', '269744', '269747', '269758', '269759', '269760']
    nosy_count = 5.0
    nosy_names = ['terry.reedy', 'vstinner', 'meador.inge', 'python-dev', 'serhiy.storchaka']
    pr_nums = []
    priority = 'normal'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'enhancement'
    url = 'https://bugs.python.org/issue26765'
    versions = ['Python 3.6']

    @serhiy-storchaka
    Copy link
    Member Author

    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.

    @serhiy-storchaka serhiy-storchaka added interpreter-core (Objects, Python, Grammar, and Parser dirs) type-feature A feature request or enhancement labels Apr 15, 2016
    @meadori
    Copy link
    Member

    meadori commented Apr 17, 2016

    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?

    @serhiy-storchaka
    Copy link
    Member Author

    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.

    1. 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.

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented May 4, 2016

    New changeset 41969033eb9d by Serhiy Storchaka in branch 'default':
    Issue bpo-26765: Moved common code and docstrings for bytes and bytearray methods
    https://hg.python.org/cpython/rev/41969033eb9d

    @serhiy-storchaka
    Copy link
    Member Author

    Here is the second part of the patch. It moves common code for replace() method to template file Objects/stringlib/transmogrify.h.

    @meadori
    Copy link
    Member

    meadori commented May 5, 2016

    LGTM.

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented May 5, 2016

    New changeset f4406d746d27 by Serhiy Storchaka in branch 'default':
    Issue bpo-26765: Moved common code for the replace() method of bytes and bytearray
    https://hg.python.org/cpython/rev/f4406d746d27

    @serhiy-storchaka
    Copy link
    Member Author

    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.

    @serhiy-storchaka
    Copy link
    Member Author

    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?

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented May 16, 2016

    New changeset c6c1882ecc77 by Serhiy Storchaka in branch 'default':
    Issue bpo-26765: Ensure that bytes- and unicode-specific stringlib files are used
    https://hg.python.org/cpython/rev/c6c1882ecc77

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Jul 1, 2016

    New changeset b0087e17cd5e by Serhiy Storchaka in branch 'default':
    Issue bpo-26765: Moved wrappers for bytes and bytearray methods to common header
    https://hg.python.org/cpython/rev/b0087e17cd5e

    @terryjreedy
    Copy link
    Member

    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 bpo-27007 and the test suite runs without error.

    @terryjreedy
    Copy link
    Member

    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. bpo-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.

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Jul 3, 2016

    New changeset 0638047c0c36 by Serhiy Storchaka in branch 'default':
    Issue bpo-26765: Fixed parsing Py_ssize_t arguments on 32-bit Windows.
    https://hg.python.org/cpython/rev/0638047c0c36

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Jul 3, 2016

    New changeset 6c4c0a23fabb by Serhiy Storchaka in branch 'default':
    Backed out changeset b0087e17cd5e (issue bpo-26765)
    https://hg.python.org/cpython/rev/6c4c0a23fabb

    @serhiy-storchaka
    Copy link
    Member Author

    I don't know what is wrong with this patch. Backed out just for the case.

    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    interpreter-core (Objects, Python, Grammar, and Parser dirs) type-feature A feature request or enhancement
    Projects
    None yet
    Development

    No branches or pull requests

    3 participants