Fixes most of Kotlin runtime tests #8
Conversation
| @@ -959,7 +959,7 @@ public open class <lexer.name>(input: CharStream) : <superClass; null="Lexer">(i | |||
| public object Modes { | |||
| public const val DEFAULT_MODE: Int = 0 | |||
| <if(rest(lexer.modes))> | |||
| <rest(lexer.modes):{m | public const val <m>: Int = <i>}; separator="\n"> | |||
| <rest(lexer.modes):{m | public const val MODE_<m; format="upper">: Int = <i>}; separator="\n"> | |||
There was a problem hiding this comment.
Just wondering why this is prefixed with MODE_ now.
There was a problem hiding this comment.
Probably this is not necessary. If the name is uppercase it should not clash with keywords anyway
There was a problem hiding this comment.
@ftomassetti yup, that's the same logic we use for the other "namespaces".
There was a problem hiding this comment.
I think we should either:
- remove the
_MODEsuffix fromDEFAULT_MODEand theMODE_prefix from the other modes - keep
DEFAULT_MODEand changeMODE_<m; format="upper">into<m; format="upper">_MODE
What do you think @lppedd ? Any preference?
There was a problem hiding this comment.
@ftomassetti I'd say the most idiomatic is option 1
remove the MODE suffix from DEFAULT_MODE and the MODE prefix from the other modes
The suffix (or prefix) is redundant inside a Modes namespace imo.
You can keep <m; format="upper"> tho, so we are sure they're always uppercased and no conflict will ever arise.
Modes will always require an uppercase initial character at the .g4 level right?
There was a problem hiding this comment.
Then I will remove the prefix MODE_ and add a separate issue for renaming DEFAULT_MODE as it would impact several files and I think this PR should be kept minimal
| @@ -84,7 +84,7 @@ public Map<Character, String> getTargetCharValueEscape() { | |||
| public String getTokenTypeAsTargetLabel(Grammar g, int ttype) { | |||
| // All tokens are namespaced inside a Tokens object. | |||
| // Here we simply force the qualification | |||
| return "Tokens." + super.getTokenTypeAsTargetLabel(g, ttype); | |||
| return super.getTokenTypeAsTargetLabel(g, ttype); | |||
There was a problem hiding this comment.
Nice one with this one. I'd just remove this method at this point.
Signed-off-by: Federico Tomassetti <federico@strumenta.com>
ftomassetti
force-pushed
the
integrate-kotlin-runtime
branch
from
c59cac6
to
311ee89
Compare
Signed-off-by: Federico Tomassetti <federico@strumenta.com>
ftomassetti
force-pushed
the
integrate-kotlin-runtime
branch
from
5cf25c9
to
98f2e34
Compare
|
Thanks, amazing work! I'll review and merge close to this weekends. |
|
Is it ready to merge? |
|
Yes, for me it is |
This is a PR on the PR #6
I tried to fix as many failing tests as possible, going from 36 to 3.
The test fixed were fixed by revising escaping rules, and by renaming getters not to clash with auto-generated getters for properties.