tjdevries / vim9jit

a vim9script -> lua transpiler (written in Rust)
MIT License
510 stars 21 forks source link

Refactor FuncLexr to use iterator functions #39

Closed johnhuichen closed 5 months ago

johnhuichen commented 8 months ago

I am making a minor change to crates/vimfuncs/build.rs.

It doesn't impact the functionality but use more idiomatic iterator. I also made some changes to keep abstraction level consistent. Please let me know if that's too many changes.

Changes:

  1. FuncLexer only has one field chars which is an iterator
  2. FuncToken has a new special token Skip
  3. FuncLexer::lex function delegates processing of characters to other struct methods
  4. Removed enum methods for FuncToken (string, number...) and instead use pattern matching to get enum values

Testing:

  1. cargo run test is green
  2. I used data/global_functions.txt to generate a list of tokens using the original script and the refactored script. They produced identifical content
tjdevries commented 8 months ago

There's kind of a lot of extra changes in the PR. It's not a huge deal but in general it's nice if the changes can be made smaller so as to be more easily reviewed (for example, some of the name changes and things I don't really like as much as the names that were there before)

johnhuichen commented 8 months ago

There's kind of a lot of extra changes in the PR. It's not a huge deal but in general it's nice if the changes can be made smaller so as to be more easily reviewed (for example, some of the name changes and things I don't really like as much as the names that were there before)

100% agreed. I started making too many changes as part of refactoring.

johnhuichen commented 8 months ago

I cleaned the changes up by

  1. renaming skip_whitespaces to skip_whitespace
  2. revert deletion of FuncToken methods
  3. revert changes to anything outside FuncLexer

I made a new change so that now lex method iterates through characters by peek() instead of next(). See L64. I think this would make the design more flexible to change in the future. Let me know what you think

@tjdevries