Skip to content

Simplify parsing ceildiv/floordiv/mod - #17

Open
shawwn wants to merge 1 commit into
spcl:masterfrom
shawwn:2022-05-15/simplify
Open

Simplify parsing ceildiv/floordiv/mod#17
shawwn wants to merge 1 commit into
spcl:masterfrom
shawwn:2022-05-15/simplify

Conversation

@shawwn

@shawwn shawwn commented May 15, 2022

Copy link
Copy Markdown

I haven't been able to reproduce the problem described in these comments:

        # Pre-transform code to avoid parsing issues with ceildiv/floordiv/mod,
        # in which two symbols could be parsed as one legal symbol (due to
        # ignoring whitespace): "d0floordivs0"

This PR removes this transformation step.

I did run into a similar problem where arith.constant 0 : index was being parsed as arith.constant0. The root issue was in bare_id. I solved it by inlining the regexes, and introducing plain_id:

// Identifier syntax
//bare_id : (letter| underscore) (letter|digit|underscore|id_chars)*
plain_id : /[a-zA-Z_][a-zA-Z0-9_$]*/
bare_id : /[a-zA-Z_][a-zA-Z0-9_$.]*/
suffix_id : digits | bare_id

... and then replacing each instance of bare_id "." bare_id with plain_id "." plain_id. I can submit that as a PR if you'd like.

@spcl spcl deleted a comment from fioushen May 23, 2022
@codecov-commenter

codecov-commenter commented May 23, 2022

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 94.43%. Comparing base (88b81ab) to head (32108aa).
⚠️ Report is 14 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master      #17      +/-   ##
==========================================
- Coverage   94.44%   94.43%   -0.01%     
==========================================
  Files          15       15              
  Lines        1782     1779       -3     
==========================================
- Hits         1683     1680       -3     
  Misses         99       99              

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@tbennun

tbennun commented May 23, 2022

Copy link
Copy Markdown
Contributor

Thanks for the PR!
Interesting, I added this change specifically because things did not work. Could you please add a test that checks for it?

As for your other issue, I'm not sure if that is the best fix, but let's start another PR with a test and your fix and I'll try it out.

@tbennun

tbennun commented Jun 20, 2022

Copy link
Copy Markdown
Contributor

@shawwn ping

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.

3 participants