Skip to content

bpo-25643: Refactor the C tokenizer#25050

Merged
pablogsal merged 6 commits into
python:masterfrom
pablogsal:better_token
Mar 28, 2021
Merged

bpo-25643: Refactor the C tokenizer#25050
pablogsal merged 6 commits into
python:masterfrom
pablogsal:better_token

Conversation

@pablogsal

@pablogsal pablogsal commented Mar 28, 2021 •
edited
Loading

Copy link
Copy Markdown
Member

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

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

Copy link
Copy Markdown
Contributor

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.

Comment thread Parser/tokenizer.c
Comment thread Parser/tokenizer.c Outdated
Comment thread Parser/tokenizer.c Outdated
Comment thread Parser/tokenizer.c
Comment thread Parser/tokenizer.c Outdated
Comment thread Parser/tokenizer.c Outdated
Comment thread Parser/tokenizer.c Outdated
Comment thread Parser/tokenizer.c Outdated
Comment thread Parser/tokenizer.c Outdated
Comment thread Parser/tokenizer.c Outdated
pablogsal and others added 3 commits March 28, 2021 22:28
@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

pablogsal commented Mar 28, 2021 •
edited
Loading

Copy link
Copy Markdown
Member Author

@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

pablogsal commented Mar 28, 2021

Copy link
Copy Markdown
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
Comment thread Parser/tokenizer.c Outdated
@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 reviewed changes

@erlend-aasland erlend-aasland left a comment

Copy link
Copy Markdown
Contributor

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