search

antlr / antlr5

BSD 3-Clause "New" or "Revised" License
57 stars 5 forks source link

Use Kotlin core for java runtime #40

Closed ericvergnaud closed 9 months ago

ericvergnaud commented 9 months ago

This is far from being mergeable (many tests fail) but I thought it might be worth starting to look at the changes and provide comments.

lppedd commented 9 months ago

Quickly skimmed through the changes. Since I suppose at some point ANTLR 5 will get rid of the Java runtime, I'd say it's all good as long as tests pass.

Just a couple of commits I'd revisit: e8ae531, 9c125e4
Better not make stuff nullable to avoid all those !! assertions. I recall in the Strumenta repository I used an ad-hoc object to replace a null value.

ericvergnaud commented 9 months ago

Quickly skimmed through the changes. Since I suppose at some point ANTLR 5 will get rid of the Java runtime, I'd say it's all good as long as tests pass.

Just a couple of commits I'd revisit: e8ae531, 9c125e4 Better not make stuff nullable to avoid all those !! assertions. I recall in the Strumenta repository I used an ad-hoc object to replace a null value.

Actually the nullables are required to make the tests pass. That said your suggestions are welcome and we might change that in favor of your proposed strategy. I'd be in favor of doing that in a separate PR such that we know what we're breaking and we can measure the impact on performance ? See #44

ericvergnaud commented 9 months ago

@KvanTTT @lppedd All tests pass !!! Your comments are welcome. Changing the title of the PR.

ericvergnaud commented 9 months ago

@KvanTTT given the broad scope of this PR, I had to merge your latest changes from dev into this branch. It wasn't seamless, so you might want to double-check the affected files.

ericvergnaud commented 9 months ago

@KvanTTT any comments ?

KvanTTT commented 9 months ago

No, I'm on vacation. Feel free to merge without my approval.