Skip to content

bpo-25643: Refactor the C tokenizer #25050

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
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
Loading

https://bugs.python.org/issue25643

Based on the original patch by Serhiy Storchaka

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

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

pablogsal and others added 3 commits March 28, 2021 22:28
@pablogsal
Copy link
Member Author

pablogsal commented Mar 28, 2021 •
edited
Loading

@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

@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 reviewed changes
@pablogsal
Update Parser/tokenizer.c
6ddb715 
Co-authored-by: Erlend Egeberg Aasland <[email protected]>
erlend-aasland
erlend-aasland approved these changes Mar 28, 2021
View reviewed changes
Copy link
Contributor

@erlend-aasland erlend-aasland left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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
@pablogsal pablogsal deleted the better_token branch March 28, 2021 22:48
@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 erlend-aasland approved these changes

@lysnikolaou lysnikolaou Awaiting requested review from lysnikolaou

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

Successfully merging this pull request may close these issues.
4 participants
@pablogsal @erlend-aasland @the-knights-who-say-ni @bedevere-bot