Skip to content

get rid of the TokenError field in Token#17

Merged
c42f merged 1 commit intoJuliaLang:mainfrom
KristofferC:kc/remove_err_field
Mar 9, 2022
Merged

get rid of the TokenError field in Token#17
c42f merged 1 commit intoJuliaLang:mainfrom
KristofferC:kc/remove_err_field

Conversation

@KristofferC
Copy link
Copy Markdown
Member

@KristofferC KristofferC commented Feb 23, 2022

It is very rare for tokens to be errors but we still need to pay the cost of the TokenError field every time. This PR just moves the TokenError into a Kind and has a function iserror to check if the kind is of an error type.

Made on top of #16

@KristofferC
Copy link
Copy Markdown
Member Author

Rebased

Copy link
Copy Markdown
Member

@c42f c42f left a comment

Choose a reason for hiding this comment

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

Looks good. I think is_error(head::SyntaxHead) may need fixing after this, and I feel slightly worried by the special case in untokenize.

Somewhat unrelated — you'll notice that the way I've bridged the parser with the lexer is hardly ideal in terms of repetition and consistency, especially for kinds. I hope to clean this up at some point by changing the way kinds are declared, probably moving away from @enum and toward the string macro approach. When parsing, it's extremely helpful to have the kind names be self representing (ie, the kind of for is K"for").

Comment thread src/parse_stream.jl

function untokenize(head::SyntaxHead; unique=true, include_flag_suff=true)
str = untokenize(kind(head); unique=unique)
str = is_error(kind(head)) ? "error" : untokenize(kind(head); unique=unique)
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.

It seems a potentially dubious to special case this in here rather than in untokenize(::Kind). I assume something fails somehow without this?

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.

Right now, the parser expects any error to untokenize to "error". But now we have many error kinds. I could just add all of them to https://github.com/c42f/JuliaSyntax.jl/blob/77b4044218a7c7c5e34f8b1459a88fe6d405b64c/src/token_kinds.jl#L6 and have them sting to "error".

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.

Yes I think they should be added to _str_to_kind — this allows them to be accessed with the K macro.

@c42f
Copy link
Copy Markdown
Member

c42f commented Mar 9, 2022

I think let's merge this. I'll add some cleanup to is_error and untokenize() in a follow-up.

@c42f c42f merged commit f310f60 into JuliaLang:main Mar 9, 2022
@KristofferC KristofferC deleted the kc/remove_err_field branch March 9, 2022 06:08
c42f pushed a commit to JuliaLang/julia that referenced this pull request Oct 17, 2025
topolarity pushed a commit to JuliaLang/julia that referenced this pull request Nov 14, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants