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

Jump to bottom

bpo-25643: Refactor the C tokenizer #25050

Merged
merged 6 commits into from Mar 28, 2021
Merged

bpo-25643: Refactor the C tokenizer #25050

merged 6 commits into from Mar 28, 2021

Conversation

pablogsal
Copy link
Member

@pablogsal pablogsal commented Mar 28, 2021 •
edited

https://bugs.python.org/issue25643

Based on the original patch by Serhiy Storchaka

@pablogsal pablogsal requested a review from lysnikolaou as a code owner Mar 28, 2021
@pablogsal pablogsal force-pushed the better_token branch 3 times, most recently from f57fa09 to 068e38a Compare Mar 28, 2021
erlend-aasland
erlend-aasland reviewed Mar 28, 2021
View changes
Copy link
Contributor

@erlend-aasland erlend-aasland left a comment •
edited

@pablogsal, @serhiy-storchaka: I added some PEP 7 comments, some comments about deprecated memory functions, and some decref / free control questions. Hope you don't mind.

Parser/tokenizer.c Show resolved Hide resolved
Parser/tokenizer.c Outdated Show resolved Hide resolved
Parser/tokenizer.c Outdated Show resolved Hide resolved
Parser/tokenizer.c Show resolved Hide resolved
Parser/tokenizer.c Outdated Show resolved Hide resolved
Parser/tokenizer.c Outdated Show resolved Hide resolved
Parser/tokenizer.c Outdated Show resolved Hide resolved
Parser/tokenizer.c Outdated Show resolved Hide resolved
Parser/tokenizer.c Outdated Show resolved Hide resolved
Parser/tokenizer.c Outdated Show resolved Hide resolved
pablogsal and others added 3 commits Mar 28, 2021
@pablogsal
Apply suggestions from code review
5c557a7 
Co-authored-by: Erlend Egeberg Aasland <erlend.aasland@innova.no>
@pablogsal
Apply suggestions from code review
9e3d4aa 
Co-authored-by: Erlend Egeberg Aasland <erlend.aasland@innova.no>
@pablogsal
Reorg the error paths
71cdeed
@pablogsal
Copy link
Member Author

pablogsal commented Mar 28, 2021 •
edited

@pablogsal, @serhiy-storchaka: I added some PEP 7 comments, some comments about deprecated memory functions, and some decref / free control questions. Hope you don't mind.

Thanks a lot for your comments and for the review. I have applied most of the PEP7 and made some changes to error returning paths so they are a bit more clear.

Regarding PEP7, normally we only adapt it when we are reworking things, but given that this change was quite big I had opted to not make it worse by changing the surrounding style, but I think it doesn't hurt that much.

@pablogsal
Copy link
Member Author

pablogsal commented Mar 28, 2021

@erlend-aasland Once you are ok with the changes, you can approve the PR if you wish :)

erlend-aasland
erlend-aasland reviewed Mar 28, 2021
View changes
Parser/tokenizer.c Outdated Show resolved Hide resolved
@pablogsal
Update Parser/tokenizer.c
6ddb715 
Co-authored-by: Erlend Egeberg Aasland <erlend.aasland@innova.no>
erlend-aasland
erlend-aasland approved these changes Mar 28, 2021
View changes
Copy link
Contributor

@erlend-aasland erlend-aasland left a comment

@erlend-aasland Once you are ok with the changes, you can approve the PR if you wish :)

LGTM :)

@pablogsal pablogsal merged commit 261a452 into python:master Mar 28, 2021
10 checks passed
@pablogsal pablogsal deleted the better_token branch Mar 28, 2021
kreathon pushed a commit to kreathon/cpython that referenced this pull request May 2, 2021
@pablogsal @kreathon
bpo-25643: Refactor the C tokenizer into smaller, logical units (pyth…
69ae406 
…onGH-25050)
@zware zware mentioned this pull request Jun 28, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@erlend-aasland erlend-aasland

@lysnikolaou lysnikolaou

Assignees
No one assigned
Labels
skip news
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@pablogsal @erlend-aasland @the-knights-who-say-ni @bedevere-bot