Skip to content

bpo-44317: Improve tokenizer errors with more informative locations#26555

Merged
pablogsal merged 5 commits into
python:mainfrom
pablogsal:bpo-44317
Jul 10, 2021
Merged

bpo-44317: Improve tokenizer errors with more informative locations#26555
pablogsal merged 5 commits into
python:mainfrom
pablogsal:bpo-44317

Conversation

@pablogsal

@pablogsal pablogsal commented Jun 5, 2021

Copy link
Copy Markdown
Member

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
}
int offset = (int)PyUnicode_GET_LENGTH(errtext);

if (col_offset == 0) {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

What if error occurred at the beginning of the line?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

What if error occurred at the beginning of the line?

Oh, I was thinking here about line numbers. This should have been -1. Thanks for the catch

Comment thread Parser/tokenizer.c Outdated
/* Number */
if (isdigit(c)) {
if (c == '0') {
const char* number_start = tok->cur;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Cat tok->start be used instead of a new variable?

Comment thread Parser/tokenizer.c Outdated
tok_backup(tok, c);
if (isdigit(c)) {
// Move to the actual current token that is incorrect
tok_nextc(tok);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Same as above, tok_nextc() cancels tok_backup().

Comment thread Parser/tokenizer.c
}
c = tok_nextc(tok);
}
char* zeros_end = tok->cur;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Cat tok->start be used instead of a new variable?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Not here because we want to highlight only the zeros and tok->cur points at the end of the number

Comment thread Parser/tokenizer.c Outdated
pablogsal and others added 2 commits June 7, 2021 10:36
Co-authored-by: Serhiy Storchaka <storchaka@gmail.com>
Comment thread Parser/tokenizer.c Outdated
@github-actions

github-actions Bot commented Jul 9, 2021

Copy link
Copy Markdown

This PR is stale because it has been open for 30 days with no activity.

@github-actions github-actions Bot added the stale Stale PR or inactive for long period of time. label Jul 9, 2021
@pablogsal pablogsal closed this Jul 10, 2021
@pablogsal pablogsal reopened this Jul 10, 2021
@pablogsal
pablogsal merged commit f24777c into python:main Jul 10, 2021
@pablogsal
pablogsal deleted the bpo-44317 branch July 10, 2021 00:29
@pablogsal pablogsal added the needs backport to 3.10 only security fixes label Jul 10, 2021
@miss-islington

Copy link
Copy Markdown
Contributor

Thanks @pablogsal for the PR 🌮🎉.. I'm working now to backport this PR to: 3.10.
🐍🍒⛏🤖

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Jul 10, 2021
…ythonGH-26555)

(cherry picked from commit f24777c)

Co-authored-by: Pablo Galindo Salgado <Pablogsal@gmail.com>
@bedevere-bot bedevere-bot removed the needs backport to 3.10 only security fixes label Jul 10, 2021
@bedevere-bot

Copy link
Copy Markdown

GH-27079 is a backport of this pull request to the 3.10 branch.

pablogsal added a commit that referenced this pull request Jul 10, 2021
…H-26555) (GH-27079)

(cherry picked from commit f24777c)

Co-authored-by: Pablo Galindo Salgado <Pablogsal@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

stale Stale PR or inactive for long period of time.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants