TOML Cleanup and Improvements #4565
Conversation
| ), | ||
| @ActionReference( | ||
| path = "Loaders/text/x-toml/Actions", | ||
| id = @ActionID(category = "Edit", id = "org.openide.actions.PushAction"), |
There was a problem hiding this comment.
Typo?
| id = @ActionID(category = "Edit", id = "org.openide.actions.PushAction"), | |
| id = @ActionID(category = "Edit", id = "org.openide.actions.PasteAction"), |
| } | ||
| return c; | ||
| } | ||
|
|
||
| //Marks are for buffering in ANTLR4, we do not really need them |
There was a problem hiding this comment.
If I read the documentation correctly, we actually need buffering/seekability of the consumed stream (the documentation of CharStream declares this as required for lookahead), but the LexerInput already handles that - isn't it?
Reading further you actually buffer the whole file in memory, which kind of defeats the purpose of the CharStream implementation. getText can only look back to places, that are protected by a mark, so the buffer is limited (assuming limited lookahead and marking).
There was a problem hiding this comment.
LexerInput readText, readLength is scoped for the actual token being processed. CharStream getIndex and getText are work on stream level. Also getText is kind of an optional method. Fortunately it used in all Lexers. Just discovered to have a problem with the previos implementation on the Antlr lexer.
I'm tempted to look around the Lexing API and probably add a more ANTLR friendly interface. Will, see. I do not think that this would be the final implementation. It is kind of good enough for now.
There was a problem hiding this comment.
Seeing, that ANTLR does not bother to implement its own interface (ANTLRInputStream) in an efficient way, I can't argue, that this implementation is inefficient, so ignore that.
For the tempation to change the lexer API to be "ANTLR" friedly: before going there, make sure you have a very good reason, at some point ANTLR will go away and we will retain the fallout.
There was a problem hiding this comment.
Well, throwing out unmarked sections of StringBuilder could be implemented one day with mark supported.
Looked around the Lexing API yesterday. Accessing the underlying buffers is not as easy as I've thought.
|
|
||
| LexerState(org.tomlj.internal.TomlLexer lexer) { | ||
| LexerState(org.tomlj.internal.TomlLexer lexer) throws IllegalArgumentException, IllegalAccessException { |
There was a problem hiding this comment.
Using reflection is an implementation detail here - I would catch IllegalAccessException and wrap it into a RuntimeException. In the future you hopefully have access to the lexer state and thus the exceptions will not be thrown.
lkishalmi
force-pushed
the
toml-cleanup
branch
from
0fa326a
to
987ec7d
Compare
Well, this one got a bit fatter I've initially planned.
It has the same cleanup applied for Yaml and Dockerfile support.
Additionally I've found the root cause of the IndexOutOfBoundException as the Lexer state was not fully saved and restored. Had to do an ugly reflection hack for that, but seems to work.
As a second commit I added the parser Syntax Error output to the UI.