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

core: Correctly parse identifiers that start with a number [Minor] #1023

Merged
merged 2 commits into from May 31, 2023

Conversation

AntonLydike
Copy link
Collaborator

This change makes it so 2d-grid is tokenized to [IDENT(2d-grid)] instead of [NUMBER(2), IDENT(d-grid)]

@AntonLydike AntonLydike self-assigned this May 30, 2023
@AntonLydike AntonLydike added xdsl xdsl framework specific changes minor For minor PRs, easy and quick to review, quickly mergeable labels May 30, 2023
@codecov
Copy link

codecov bot commented May 30, 2023

Codecov Report

Patch and project coverage have no change.

Comparison is base (62f0630) 87.00% compared to head (087105e) 87.00%.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #1023   +/-   ##
=======================================
  Coverage   87.00%   87.00%           
=======================================
  Files         125      125           
  Lines       19202    19197    -5     
  Branches     2915     2914    -1     
=======================================
- Hits        16706    16702    -4     
+ Misses       2000     1999    -1     
  Partials      496      496           
Impacted Files Coverage Δ
tests/test_pass_lexer.py 100.00% <ø> (ø)
xdsl/utils/parse_pipeline.py 100.00% <ø> (ø)

... and 1 file with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

Copy link
Collaborator

@webmiche webmiche left a comment

Choose a reason for hiding this comment

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

This does not really generalize, right? So 13d-grid has still the same issue. This feels quite intuitive, would there be an easy fix?

@AntonLydike
Copy link
Collaborator Author

This does not really generalize, right? So 13d-grid has still the same issue. This feels quite intuitive, would there be an easy fix?

It does actually handle this! [0-9]+[A-Za-z_-]+[A-Za-z0-9_-]* matches one or more numbers, then at least one non-number character, and then any amount of "normal" ident characters.

@PapyChacal
Copy link
Collaborator

This does not really generalize, right? So 13d-grid has still the same issue. This feels quite intuitive, would there be an easy fix?

It does actually handle this! [0-9]+[A-Za-z_-]+[A-Za-z0-9_-]* matches one or more numbers, then at least one non-number character, and then any amount of "normal" ident characters.

Let's add tests then?

Copy link
Collaborator

@math-fehr math-fehr left a comment

Choose a reason for hiding this comment

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

👍

Copy link
Collaborator

@webmiche webmiche left a comment

Choose a reason for hiding this comment

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

Okay, that's fine then 👍
Not used to python regex syntax 😅

@superlopuh
Copy link
Member

Do you know what the MLIR regex is by any chance?

@AntonLydike
Copy link
Collaborator Author

Do you know what the MLIR regex is by any chance?

MLIR uses the llvm infrastructure, and that doesn't use regexes. It's a really convoluted system for them.

@AntonLydike AntonLydike merged commit 81d3513 into main May 31, 2023
12 checks passed
@AntonLydike AntonLydike deleted the anton/pass-args-fix-ident branch May 31, 2023 10:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
minor For minor PRs, easy and quick to review, quickly mergeable xdsl xdsl framework specific changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants