Cleanup the runtime code #114

lppedd · 2023-12-07T20:36:33Z

Goals

Improving the runtime performance by avoiding unnecessary (unchecked) casts, unnecessary non-null assertions, by using the right data structures (possibly the same used in the Java runtime) and immutable types
Improving the development and debugging experience
Improving the IDE navigation experience
Easing the process of porting future changes

Non goals

It is a non goal to change the behavior of the code. It should possibly stay aligned with the Java runtime, line by line.

Methodology

Each commit is mapped to a runtime Kotlin package.

Each file is cleaned-up in multiple passes:

A first pass is done using the IDE formatter
A second pass is done by resolving all possible inspections
A third pass is done by matching the file, line by line, with the Java runtime equivalent.
If a method is missing (e.g., StringBuilder.appendCodePoint), it is implemented.

TODO

Calls to toHexString should be passed a custom hex format to align to the Java implementation.

val hexFormat = HexFormat {
 upperCase = true
 number {
   prefix = "0x"
   removeLeadingZeros = true
 }
}

Cleanup calls to System.out.println
The Java runtime calls .format with a terminating \n, but we don't need that \n

antlr-kotlin-runtime/src/commonMain/kotlin/org/antlr/v4/kotlinruntime/misc/OrderedHashSet.kt

antlr-kotlin-runtime/src/commonMain/kotlin/com/strumenta/kotlinmultiplatform/Arrays.kt

antlr-kotlin-runtime/src/commonMain/kotlin/org/antlr/v4/kotlinruntime/Lexer.kt

lppedd · 2023-12-12T09:22:09Z

@ftomassetti I'm going to finish off the main package and then call it done.
There will be org.antlr.v4.kotlinruntime.atn still not cleaned up but I can do it in the future.

ftomassetti · 2023-12-12T12:55:31Z

antlr-kotlin-runtime/src/commonMain/kotlin/org/antlr/v4/kotlinruntime/misc/Array2DHashSet.kt

+  override fun isEmpty(): Boolean =
+    n == 0
+
+  // TODO(Edoardo): why is this one commented out?


TODO that we should look into before merging

Answer is we already have the typed version.

override fun contains(element: T): Boolean = containsFast(element)

So I've simply removed it now.

ftomassetti · 2023-12-12T12:56:16Z

antlr-kotlin-runtime/src/commonMain/kotlin/org/antlr/v4/kotlinruntime/misc/FlexibleHashMap.kt

+      "${key}:${value}"
+  }
+
+  // TODO: are bucket entries actually nullable?


ftomassetti · 2023-12-12T12:56:36Z

antlr-kotlin-runtime/src/commonMain/kotlin/org/antlr/v4/kotlinruntime/misc/FlexibleHashMap.kt

+      existingBucket
+    } else {
+      // TODO(Edoardo): should be a LinkedList
+      val list = ArrayList<Entry<K, V>?>()


Can't do anything about it right now. Behavior is the same with ArrayList luckily.
Kotlin Multiplatform needs a common LinkedList, at that point we can switch.

ftomassetti · 2023-12-12T12:57:53Z

antlr-kotlin-runtime/src/commonMain/kotlin/org/antlr/v4/kotlinruntime/misc/IntegerList.kt

 * @author Sam Harwell
 */
-open class IntegerList {
+public open class IntegerList {


In Kotlin, ca we use lists of Ints?

Let's keep the runtime a 1 to 1 match. I'd prefer using IntegerList instead of changing all the usages.

Also, using a List<Int> means dealing with objects instead of primitive ints.

ftomassetti · 2023-12-12T12:58:06Z

...otlin-runtime/src/commonMain/kotlin/org/antlr/v4/kotlinruntime/misc/InterpreterDataReader.kt

@@ -9,17 +9,16 @@ package org.antlr.v4.kotlinruntime.misc
 import org.antlr.v4.kotlinruntime.Vocabulary
 import org.antlr.v4.kotlinruntime.atn.ATN

-
+// TODO(Edoardo): this one requires reading files


I will implement this for the JVM only probably, but on a subsequent PR, not this one, don't want to mix too many changes.

ftomassetti · 2023-12-12T12:58:43Z

antlr-kotlin-runtime/src/commonMain/kotlin/org/antlr/v4/kotlinruntime/misc/LogManager.kt

-class LogManager {
-
+// TODO(Edoardo): uncomment once kotlinx-datetime supports date formatting
+public class LogManager {


Should we move this to a ticket and delete the file?

No no, let's keep the file there so we know we need to look into it.
kotlinx-datetime will get formatting support in a couple weeks I think.

ftomassetti · 2023-12-12T12:59:22Z

antlr-kotlin-runtime/src/commonMain/kotlin/org/antlr/v4/kotlinruntime/misc/MurmurHash.kt

@@ -10,99 +10,90 @@ package org.antlr.v4.kotlinruntime.misc
 *
 * @author Sam Harwell
 */
-object MurmurHash {
+public object MurmurHash {


What is a MurmurHash?

See https://en.wikipedia.org/wiki/MurmurHash

ftomassetti · 2023-12-12T13:00:58Z

I started taking a look at the code, but I am afraid I will not be able to do a proper review as there are too many changes. Also, I think this is code coming from the ANTLR Java runtime, so I assume that most of it will be correct. And I will rely on tests to catch blatant things

lppedd · 2023-12-12T13:02:28Z

Definitely don't review it in details, not worth it.
The code is a 1 to 1 mapping with the Java runtime (I'm doing it by reading each source file line by line), with some Kotlin-friendly code style changes.

lppedd · 2023-12-13T16:45:11Z

I'm almost done. Just missing a couple of big classes.

In the end I did also the atn package, as the type nullability is tied to the parser and lexer.

…clash

lppedd · 2023-12-14T00:04:57Z

Done!

Turns out there was code not on par with ANTLR 4.13.1.
I've also re-enabled functionalities which were commented out, such as ParserInterpreter, ATNSerializer, XPath* (although we still miss a multiplatform implementation for a couple of Java methods), and many others.

I've deleted two files, which were commented out completely:

UnbufferedCharStream: pretty much unnecessary for us + relies on Java's Readers
LogManager: unnecessary for us + relies on Java's Readers

We might be able to restore them at some point, but now it doesn't make sense.

lppedd · 2023-12-14T00:07:34Z

I'll do a couple tests (@ftomassetti you can run some tests on your grammars too if you want), update the branch with master's changes, and then mark it as ready to be merged.

Note that we now have a very strong type system, so a release with this code is not backward compatible.

lppedd · 2023-12-14T00:19:19Z

antlr-kotlin-runtime/src/commonMain/kotlin/org/antlr/v4/kotlinruntime/tree/xpath/XPathLexer.kt

+  public fun isNameChar(@Suppress("UNUSED_PARAMETER") c: Int): Boolean {
+    TODO("Need isUnicodeIdentifierPart")
+    // return Character.isUnicodeIdentifierPart(c)
+  }
+
+  public fun isNameStartChar(@Suppress("UNUSED_PARAMETER") c: Int): Boolean {
+    TODO("Need isUnicodeIdentifierStart")
+    // return Character.isUnicodeIdentifierStart(c)
+  }


If you know how to re-implement those two methods, feel free to let me know or open another PR after this one is merged.

…to platformPrintErr

ftomassetti · 2023-12-14T13:30:10Z

Note that we now have a very strong type system, so a release with this code is not backward compatible.

Makes sense, I think at this stage breaking changes have to be expected

frett · 2023-12-14T16:07:25Z

as someone using this in production multiplatform code I'm fine with the breaking changes since it's bringing it up to be more maintainable and Kotlin friendly :)

lppedd · 2023-12-14T18:40:47Z

Cool! Hopefully there won't be changes this big anymore.

We can merge this one and publish it as another RC, so you can also test it.
I'll try to integrate more grammar tests.

lppedd · 2023-12-14T21:12:07Z

The C++ grammar from the ANTLR4 grammars repository (which is enormous), compiles fine 🔝

The not-so-good news: the C++ generated parser (20000 LOC) crashes the Kotlin compiler lol

lppedd · 2023-12-14T21:22:42Z

Managed to get it to run by replacing the scoped { } inline function with if (true).

C++ tests run fine and pass. I'll open a YouTrack issue once this is merged, since this is valid for K2 too.

lppedd · 2023-12-14T21:30:24Z

I've inspected the outputted JS code and the good news is the K/JS compiler optimizes the if (true) check away.

I'll use that one in place of scoped for now, so that it works ok even if you've got an absurdly big grammar.

ftomassetti · 2023-12-15T08:52:38Z

Great work @lppedd , should we mark the PR as ready for review and merge it?

lppedd · 2023-12-15T08:55:41Z

Thank you! Yup, let's merge this one too, but first let's merge #115

lppedd added 3 commits December 7, 2023 14:11

refactor: cleanup package org.antlr.v4.kotlinruntime.ast

349eebc

refactor: cleanup package org.antlr.v4.kotlinruntime.dfa

9e58aa9

refactor: cleanup package org.antlr.v4.kotlinruntime.misc

4082921

lppedd commented Dec 8, 2023

View reviewed changes

antlr-kotlin-runtime/src/commonMain/kotlin/org/antlr/v4/kotlinruntime/misc/OrderedHashSet.kt Outdated Show resolved Hide resolved

ftomassetti reviewed Dec 8, 2023

View reviewed changes

antlr-kotlin-runtime/src/commonMain/kotlin/com/strumenta/kotlinmultiplatform/Arrays.kt Show resolved Hide resolved

lppedd added 4 commits December 9, 2023 12:20

refactor: align the string hex representation to the Java runtime

a647e38

refactor: cleanup package org.antlr.v4.kotlinruntime.tree.pattern

c25fc4d

refactor: cleanup package org.antlr.v4.kotlinruntime.tree

bf05b25

refactor: restore Lexer.setInputStream

000feb7

lppedd commented Dec 11, 2023

View reviewed changes

antlr-kotlin-runtime/src/commonMain/kotlin/org/antlr/v4/kotlinruntime/Lexer.kt Outdated Show resolved Hide resolved

lppedd added 2 commits December 11, 2023 12:56

chore: TODO > Note

be6ca17

refactor: remove inputStream from the Recognizer class

7b761e9

ftomassetti reviewed Dec 12, 2023

View reviewed changes

lppedd added 7 commits December 14, 2023 00:30

refactor: cleanup package org.antlr.v4.kotlinruntime

f60fb94

refactor: allowing getting system environment variables

b100985

refactor: cleanup multiplatform utilities

76b7d1f

refactor: delete UnbufferedCharStream and LogManager

44ddb96

refactor: cleanup package org.antlr.v4.kotlinruntime.atn

43aca68

refactor: restore XPath

913c83c

refactor: general cleanup

aa9e8ed

lppedd added 6 commits December 14, 2023 00:41

chore: update target's template to reflect runtime changes

979968e

test: cleanup test functions

a8b3534

build: enable explicit API for the runtime

9c1ec27

chore: update .editorconfig

686a4d8

refactor: rename OrderedHashSet.elements with JsName to avoid a name …

d8db5fa

…clash

chore: format missed Predicate

cc751a6

lppedd commented Dec 14, 2023

View reviewed changes

lppedd added 2 commits December 14, 2023 12:07

refactor: reduce the number of multiplatform wrappers

3fe674e

refactor: rename printErrorLine to platformPrintErrLn and printError …

9e0fc4b

…to platformPrintErr

lppedd mentioned this pull request Dec 14, 2023

Cleanup build scripts and restore ability to publish to a private repository #115

Merged

lppedd added 3 commits December 14, 2023 21:17

build: allow turning off a single JS target (Node/Browser)

6ff56f8

build: properly setup testing frameworks for Node and browser

a37671c

chore: disambiguate kotlin.Int from enum entries named Int

f9a6c7e

lppedd added 2 commits December 14, 2023 22:32

chore: use 'if (true) {}' instead of the scoped function

c582bfe

chore: further beautify generated code

68333ae

lppedd marked this pull request as ready for review December 15, 2023 15:40

ftomassetti merged commit bc7d414 into Strumenta:master Dec 18, 2023
3 checks passed

lppedd deleted the chore/formatting branch December 18, 2023 10:51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Cleanup the runtime code #114

Cleanup the runtime code #114

lppedd commented Dec 7, 2023 •

edited

Loading

lppedd commented Dec 12, 2023

ftomassetti Dec 12, 2023

lppedd Dec 12, 2023

ftomassetti Dec 12, 2023

ftomassetti Dec 12, 2023

lppedd Dec 12, 2023

ftomassetti Dec 12, 2023

lppedd Dec 12, 2023 •

edited

Loading

ftomassetti Dec 12, 2023

lppedd Dec 12, 2023

ftomassetti Dec 12, 2023

lppedd Dec 12, 2023

ftomassetti Dec 12, 2023

lppedd Dec 12, 2023

ftomassetti commented Dec 12, 2023

lppedd commented Dec 12, 2023 •

edited

Loading

lppedd commented Dec 13, 2023

lppedd commented Dec 14, 2023

lppedd commented Dec 14, 2023

lppedd Dec 14, 2023

ftomassetti commented Dec 14, 2023

frett commented Dec 14, 2023

lppedd commented Dec 14, 2023

lppedd commented Dec 14, 2023 •

edited

Loading

lppedd commented Dec 14, 2023 •

edited

Loading

lppedd commented Dec 14, 2023

ftomassetti commented Dec 15, 2023

lppedd commented Dec 15, 2023

Cleanup the runtime code #114

Cleanup the runtime code #114

Conversation

lppedd commented Dec 7, 2023 • edited Loading

Goals

Non goals

Methodology

TODO

lppedd commented Dec 12, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lppedd Dec 12, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ftomassetti commented Dec 12, 2023

lppedd commented Dec 12, 2023 • edited Loading

lppedd commented Dec 13, 2023

lppedd commented Dec 14, 2023

lppedd commented Dec 14, 2023

Choose a reason for hiding this comment

ftomassetti commented Dec 14, 2023

frett commented Dec 14, 2023

lppedd commented Dec 14, 2023

lppedd commented Dec 14, 2023 • edited Loading

lppedd commented Dec 14, 2023 • edited Loading

lppedd commented Dec 14, 2023

ftomassetti commented Dec 15, 2023

lppedd commented Dec 15, 2023

lppedd commented Dec 7, 2023 •

edited

Loading

lppedd Dec 12, 2023 •

edited

Loading

lppedd commented Dec 12, 2023 •

edited

Loading

lppedd commented Dec 14, 2023 •

edited

Loading

lppedd commented Dec 14, 2023 •

edited

Loading