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.

Sign up for GitHub

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

Jump to bottom

In parsermodule.c, replace over 2KLOC of hand-crafted validation code, with a DFA #70713

Closed
tyomitch mannequin opened this issue Mar 10, 2016 · 11 comments
Closed

In parsermodule.c, replace over 2KLOC of hand-crafted validation code, with a DFA #70713

tyomitch mannequin opened this issue Mar 10, 2016 · 11 comments
Assignees
@freddrake
Labels
extension-modules C modules in the Modules dir type-feature A feature request or enhancement

Comments

@tyomitch
Copy link
Mannequin

tyomitch mannequin commented Mar 10, 2016

BPO 26526
Nosy @freddrake, @brettcannon, @gpshead, @giampaolo, @benjaminp, @berkerpeksag, @serhiy-storchaka, @tyomitch
Files
  • patch
  • issue26526_16704_63395.diff: Fixed a stray tab and expanded a comment
    		•

    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 = 'https://github.com/freddrake'
closed_at = <Date 2016-06-02.18:32:59.721>
created_at = <Date 2016-03-10.09:02:31.787>
labels = ['extension-modules', 'type-feature']
title = 'In parsermodule.c, replace over 2KLOC of hand-crafted validation code, with a DFA'
updated_at = <Date 2017-01-26.07:35:57.343>
user = 'https://github.com/tyomitch'

    bugs.python.org fields:

    activity = <Date 2017-01-26.07:35:57.343>
actor = 'python-dev'
assignee = 'fdrake'
closed = True
closed_date = <Date 2016-06-02.18:32:59.721>
closer = 'python-dev'
components = ['Extension Modules']
creation = <Date 2016-03-10.09:02:31.787>
creator = 'A. Skrobov'
dependencies = []
files = ['42110', '43069']
hgrepos = []
issue_num = 26526
keywords = ['patch']
message_count = 11.0
messages = ['261486', '265411', '266441', '266577', '266741', '266903', '266904', '272318', '272754', '286261', '286295']
nosy_count = 10.0
nosy_names = ['fdrake', 'brett.cannon', 'gregory.p.smith', 'giampaolo.rodola', 'benjamin.peterson', 'python-dev', 'berker.peksag', 'serhiy.storchaka', 'xcombelle', 'A. Skrobov']
pr_nums = []
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'enhancement'
url = 'https://bugs.python.org/issue26526'
versions = ['Python 3.6']
    @tyomitch
    Copy link
    Mannequin Author

    tyomitch mannequin commented Mar 10, 2016

    Updating Modules/parsermodule.c for every change in grammar and/or parser is a maintenance burden, listed as such at https://docs.python.org/devguide/grammar.html

    The attached patch lets the validation code use the auto-generated DFA structures, thus ensuring it stays up to date with the grammar. It also trims the code by over 2KLOC.

    @tyomitch tyomitch mannequin added extension-modules C modules in the Modules dir type-feature A feature request or enhancement labels Mar 10, 2016
    @vstinner vstinner changed the title In parsemodule.c, replace over 2KLOC of hand-crafted validation code, with a DFA In parsermodule.c, replace over 2KLOC of hand-crafted validation code, with a DFA Mar 10, 2016
    @tyomitch
    Copy link
    Mannequin Author

    tyomitch mannequin commented May 12, 2016

    Ping? This patch is two months old now.

    @freddrake
    Copy link
    Member

    I see your message to python-dev, and apologize for taking so long to get to this.

    I do intend to read through your changes, and hope to be able to make time while I'm at PyCon this coming week.

    @freddrake
    Copy link
    Member

    I've read through this, but haven't applied the patch & run tests (that's what buildbots are for).

    No objections.

    @tyomitch
    Copy link
    Mannequin Author

    tyomitch mannequin commented May 31, 2016

    Thank you Fred for your review!

    I don't have commit access myself; can anybody please commit it for me?

    @brettcannon
    Copy link
    Member

    Are you up for trying to commit this while at the sprints, Fred? If not then assign it back to me and I can eventually commit it (busy w/ a ton of stuff so I can't promise when I will get to it).

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Jun 2, 2016

    New changeset 4a9159ea2536 by Benjamin Peterson in branch 'default':
    replace custom validation logic in the parse module with a simple DFA validator (closes bpo-26526)
    https://hg.python.org/cpython/rev/4a9159ea2536

    @python-dev python-dev mannequin closed this as completed Jun 2, 2016
    @xcombelle
    Copy link
    Mannequin

    xcombelle mannequin commented Aug 10, 2016

    The DFA is generated by other part of existing cpython code ? If it's the case looks like you did a great job.

    @tyomitch
    Copy link
    Mannequin Author

    tyomitch mannequin commented Aug 15, 2016

    Thanks Xavier! Yes, this is the same DFA that's used by the main Python parser. For some reason, parsermodule didn't previously reuse it, but instead did its own thing.

    Any volunteers to review the other patch for Python parser, at http://bugs.python.org/issue26415 ?

    @tyomitch
    Copy link
    Mannequin Author

    tyomitch mannequin commented Jan 25, 2017

    Oh btw, the comment in the beginning of Grammar/Grammar

    Note: Changing the grammar specified in this file will most likely

    require corresponding changes in the parser module

    (../Modules/parsermodule.c).

    is no longer true: after this patch went in, changing the grammar no longer requires translating the changes into Modules/parsermodule.c

    Can somebody please remove the obsolete comment from there?

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Jan 26, 2017

    New changeset 3275d4b584a2 by Benjamin Peterson in branch '3.6':
    remove comment about updating the parser module; we do not need to do that anymore (bpo-26526)
    https://hg.python.org/cpython/rev/3275d4b584a2

    @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
    Assignees

    @freddrake freddrake

    Labels
    extension-modules C modules in the Modules dir type-feature A feature request or enhancement
    Projects
    None yet
    Milestone
    No milestone
    Development

    No branches or pull requests
    2 participants
    @brettcannon @freddrake