search

Strumenta / antlr-kotlin

Support for Kotlin as a target for ANTLR 4
Apache License 2.0
222 stars 47 forks source link

Java CharStreams is broken #104

Closed alchitry closed 9 months ago

alchitry commented 11 months ago

The fromChannel() function in CharStreams for the JVM is broken. It copies the entire buffer including empty spaces on each loop. It also copies it even when nothing was read causing a file that fits in the buffer to be duplicated with a bunch of null values between them.

I fixed this in my fork with this commit https://github.com/alchitry/antlr-kotlin/commit/b2514c7bd2fd2d09cfa9c3cf4e952d4286b91ea8

My fork is heavily modified (I removed multiplatform support) so it would be annoying to make a pull request from it.

I also changed up the other methods in this file to be more efficient using a BufferedReader to just read in the entire file instead of reading it in a weird loop byte by byte. https://github.com/alchitry/antlr-kotlin/blob/master/antlr-kotlin-runtime/src/main/kotlin/org/antlr/v4/kotlinruntime/CharStreams.kt

lppedd commented 9 months ago

@alchitry would it be a problem if I try out your fork? Seems you've fixed a bunch of stuff.
It would also be cool to fix #57, which basically makes the project unusable.

Edit: ah, I see you removed multiplatform.

alchitry commented 9 months ago

You're of course welcome to use/look at the fork.

My initial goal for it was to create tree walkers that could suspend and I only needed the jvm target. Removing multiplatform made this easier.

That being said, a lot of the fixes shouldn't be too hard to bring over if you need multiplatform support by looking at the commits.

The biggest changes (IIRC) were to using the newer version of ANTLR and edits to the template files.

lppedd commented 9 months ago

Thanks @alchitry! I'll probably fork and cherry pick some of your stuff.
Would you mind just pointing out how/where you updated the ANTLR version? Was it something you managed to get working without headaches?

alchitry commented 9 months ago

The version is here https://github.com/alchitry/antlr-kotlin/blob/master/buildSrc/src/main/kotlin/Versions.kt

If I remember correctly, updating it broke some things in the template file and I had to update the KotlinTarget file as you can no longer extend JavaTarget but have to extend Target.

lppedd commented 9 months ago

Perfect, thanks again.

lppedd commented 9 months ago

I was wondering, is there a specification for the ANTLR runtime? So that changes can be kept in sync for the Kotlin target too.