search

Chevrotain / chevrotain

Parser Building Toolkit for JavaScript
https://chevrotain.io
Apache License 2.0
2.44k stars 200 forks source link

feat: support pluggable lookahead strategy #1852

Closed msujew closed 1 year ago

msujew commented 1 year ago

Supersedes and closes https://github.com/Chevrotain/chevrotain/pull/1793

Allows users to supplement the parser with their own lookahead strategy. Mostly introduces new interfaces to override the existing lookahead builder functions and exposes some internals (such as getLookaheadPaths). The latter is necessary, as most lookahead strategy will still try to perform LL(k) style lookahead in one way or another. For example LL(*) needs it to identify LL(1) decisions.

The PR contains some minor updates to the documentation on this. Once this is merged and TypeFox/Langium releases their LL(*) lookahead strategy, I would update it with a reference.

Since this is basically just restructuring existing code, it wasn't necessary to write tests for it. Instead the ecma_quirks.ts tests were adapted to make use of the new API, providing an integration test for this feature.

The changes seem to have absolutely no impact on performance:

Library Ops/sec Relative Speed
JSON 9474.71 101.66%
CSS 3140.82 99.75%
ECMA5 538.65 100.02%
bd82 commented 1 year ago

Thanks for looking into the performance numbers as part of this PR. I will try to review it on the weekend. Worse case it should be reviewed on the upcoming holidays (next weekend)

msujew commented 1 year ago

@bd82 Thanks for your review. I've incorporated your comments (and left a comment where I had something to add to your comment). I agree, having an experimental namespace sounds good to me for faster iterations on the feature 👍

bd82 commented 1 year ago

Hi @msujew

I think some of the code that was removed actually does have a purpose (see comments above). Once you do so I will:

  1. re-review and hopefully merge.

And in a different PR I will:

  1. investigate possibility for a small refactor with the maxLookahead param.
  2. Move the APIs to an experimental namespace.
msujew commented 1 year ago

@bd82 I came back from vacation and integrated your comment. Good to know that the code does serve a purpose :D

Anyway, hopefully this should be good to go now.