Strumenta / antlr-kotlin

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

Regenerate XPathLexer and add test cases for XPath #151

Closed lppedd closed 7 months ago

lppedd commented 7 months ago

As per title.

ftomassetti commented 7 months ago

I think the PR makes sense. My only open comment is about enums: having them seems to me slightly better (avoid duplication of names, make possible to iterate over them). On the other hand it is slightly different from the Java runtime. While my preference is for enum, I do not feel too strongly about them, so @lppedd let me know if you want me to merge the PR as is, and I will do that

lppedd commented 7 months ago

@ftomassetti enums in generated code have never been used in the classic enum way. There was no compile-time checking on enum entries. Also, the generated code for enums tend to produce an non-insignificant overhead for the JS platform, both on code size and runtime, as the enum entries are lazily initialized and null checks are performed on each enum entry access.

For clarification purpose I'll also explain this quote.

An advantage of enums is that they avoid this duplication of names

This is not really true. Before #146, names were duplicated in the form of code:

// This has been removed in #146
<if(parser.tokens)>
<parser.tokens:{k | private val <k> = Tokens.<k>.id}; separator="\n", wrap, anchor>
<endif>

as you couldn't access the enums directly in the generated operations.

Additionally, enum complicates handling enum entry names (as #145 shows).
Keeping const val wrapped in namespaces is the better approach imo.

ftomassetti commented 7 months ago

@lppedd , ok, makes sense. Thank you for the discussion