Skip to content

Conversation

@lue-bird
Copy link
Contributor

@lue-bird lue-bird commented Jul 29, 2024

The implementation of expression optimistically pads right (and left?). This is sometimes taken for granted, replacing layout, creating a too lenient parser.
The "new" parser is now sometimes more strict (e.g. requiring if else to stay above current indent), and sometimes more lenient (e.g. allowing case(x)of(Variant)->).

I found these after converting the expression parser to a non-padding parser (which turned out to be slower so I never committed it but the corrections are still right).

In terms of performance, it's nothing much, the biggest change coming from an avoided List.map by switching from Node Expression -> Parser (Node Expression) to Parser (union type) for the pratt things.

  • before (taken from last PR): 18849.700000001118 ms to do 100 runs => 188.49700000001118 ms/run
  • after (before additional layout corrections): 16425.19999998808 ms to do 100 runs => 164.25199999988078 ms/run
  • after (with additional layout corrections): 16782.10000000894 ms to do 100 runs => 167.8210000000894 ms/run
  • after (final optimizations after that): 16077.300000000745 ms to do 100 runs => 160.77300000000744 ms/run

@jfmengels
Copy link
Collaborator

Do you think you could write tests for the code samples where we were too lenient before this PR? I'd love for the test harness to catch that in the future.

}
)
)
-- yes, ( ) is a valid pattern but not a valid type or expression
Copy link
Collaborator

@jfmengels jfmengels Jul 31, 2024

Choose a reason for hiding this comment

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

Do we have a test for this?
I think we would ideally want one for all 3 situations (patterns, types and expressions)

@jfmengels jfmengels merged commit ee38b7d into stil4m:master Jul 31, 2024
@jfmengels
Copy link
Collaborator

Really nice! And the performance improvement is nothing to scoff at either!

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