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

PTX Parser #46

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

PTX Parser #46

wants to merge 2 commits into from

Conversation

Ellipse0934
Copy link

@Ellipse0934 Ellipse0934 commented Feb 13, 2022

I don't think it looks too bad.
Looks like
image
Using the theme:

@theme CustomTheme Dict(
    :style => S"bg: f7f3ee; fg: 605b53",
    :tokens => Dict(
        TEXT    => S"",
        KEYWORD => S"fg: bf00ff; bold",
        STRING  => S"fg: a1789f",
        NAME_LABEL => S"fg: ffcc00;",
        NAME_VARIABLE => S"fg: cc1100",
        NUMBER => S"fg: 000099",
        LITERAL => S"fg: 223399"
    ),
)

My main annoyance is that the lexer isn't greedy. Given some regex like (is)|(island), the lexer will happily match is whereas I would prefer that it matches the longest possible string. Lex/GNU flex is greedy and I like that approach. Another I want to match a word: \w+ it will happily keep matching a single letter at a time, it evade this behaviour we can capture an anchor too, like \w+[ \t,\.]. However it would be more comfortable to not capture this anchor and deal with it via another rule.

@Ellipse0934
Copy link
Author

@MichaelHatherly Does my comment about the longer match make sense ? Is this something we can apply to this package ?

Here's the GNU Docs for flex

If it finds more than one match, it takes the one matching the most text (for trailing context rules, this includes the length of the trailing part, even though it will then be returned to the input). If it finds two or more matches of the same length, the rule listed first in the flex input file is chosen.

@MichaelHatherly
Copy link
Member

Is this something we can apply to this package ?

Yes, so long as it doesn't end up breaking the existing interface that would be fine to add I think.

Copy link
Member

@MichaelHatherly MichaelHatherly left a comment

Choose a reason for hiding this comment

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

Adding some tests to the test suit for this lexer would be great.

RLexer,
TOMLLexer
TOMLLexer,
Copy link
Member

Choose a reason for hiding this comment

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

Extra , added here?

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