Fix code generation by partially backporting ANTLR5 changes #167
Conversation
Signed-off-by: Federico Tomassetti <[email protected]>
|
|
||
| override fun getText(start: Token, stop: Token): String? = | ||
| getText(Interval.of(start.tokenIndex, stop.tokenIndex)) | ||
| override fun getText(start: Token?, stop: Token?): String? = |
There was a problem hiding this comment.
Could this be moved to TokenStream? Or become an extension method for TokenStream? In this way we would avoid the duplication we have in UnbufferedTokenStream
There was a problem hiding this comment.
@ftomassetti we could, but the better approach imo is to stick with simple code that is aligned with ANTLR4 Java, and ANTLR5, even if there is a small amount of duplication.
antlr-kotlin-runtime/src/commonMain/kotlin/org/antlr/v4/kotlinruntime/atn/ATNDeserializer.kt
Show resolved
Hide resolved
| override val ruleIndex: Int = Rules.<struct.derivedFromName; format="cap"> | ||
|
|
||
| <attrs:{a | public var <a>}; separator="\n"> | ||
| <attrs:{a | @JvmField public var <a>}; separator="\n"> |
There was a problem hiding this comment.
This change may break existing code, which I think is acceptable, but I just wonder if it could make sense to first replicate the tests we have in ANTLR 5, so that we are more confident this change will work on all platforms before introducing it
There was a problem hiding this comment.
Also, this avoid the platform clash on the JVM, but does it work also on other platforms?
There was a problem hiding this comment.
This change may break existing code
I think that's not something you have to take into account, as I really doubt a Java consumer is using this Kotlin target.
so that we are more confident this change will work on all platforms before introducing it
I have tested with code specifically written to reproduce conflicting signatures, and all supported platforms compiled and run correctly.
Also, this avoid the platform clash on the JVM, but does it work also on other platforms?
The problem is specific to the JVM. In JS/TS you end up with code similar to:
class Example {
get foo(): number;
getFoo(): string;
}
There was a problem hiding this comment.
@piacenti ahh no, this is what I did test, your case is a new one.
|
I will do a new release. I think that some time in the near future we should move to 1.0, as this code seems performing well, pass all tests, and be used in production by several companies, so the current version number is deceitful |
The main difference here is the use of
@JvmFieldto avoid JVM-specific conflicts (while also avoiding JS name clashes), and the addition ofruleIndexto the reserved words.ruleIndexhas to be reserved as the generated code will already have such a property:This should be merged before #166.
Closes: #162