Warn about actions not being run by the interpreter #547
Conversation
…interpreter Signed-off-by: Bastien Jansen <[email protected]>
Signed-off-by: Bastien Jansen <[email protected]>
|
|
||
| private void warnAboutPredicatesAndActions() { | ||
| if (!warnedAboutPredicates) { | ||
| notifyErrorListeners("WARNING: predicates and actions are not run by this interpreter. " + |
There was a problem hiding this comment.
I suggest using correct location of action/sempred (pass it to the method), not the first line.
There was a problem hiding this comment.
It seems to me that it's already the case:
public final void notifyErrorListeners(String msg) {
notifyErrorListeners(getCurrentToken(), msg, null);
}I can see the correct line:column in the generated message, as you can see in #523 (comment)
There was a problem hiding this comment.
I see but I'm not sure it's correct. It should point line:column in the grammar not in the input.
There was a problem hiding this comment.
I was actually thinking of a generic message saying that there are actions or predicates in the grammar. If there are multiple, I'm not sure a single line and call them from the grammar will be useful and could be misleading.
The mechanism @bjansen proposes is better in the sense that it warns only if an action/pred is seen during the parse, rather than generically if there are ever any actions visible.
@KvanTTT 's Point is still valid though that it's pointing into the input stream with getCurrentToken(). On the other hand, maybe it makes sense to simply make it more specific that upon reaching input token foo, we bypassed an action?
There was a problem hiding this comment.
it warns only if an action/pred is seen during the parse
That was indeed what I wanted to achieve.
I guess if an action is not run, there's a high probability there will be parse errors. So if we use getCurrentToken(), doesn't it make sense to "group" two error messages on the same token? One saying "hey we didn't expect this token here", and the other saying "but wait we skipped an action so it might be what's causing the error".
OTOH if we point to some location in the grammar, we're not actually "linking" the warning to any problem in the input.
There was a problem hiding this comment.
It's more likely that a predicate what caused problems but of course predicate usually don't make sense without other actions to set state. I don't think we have handy access to map an action back into the grammar file/line/column. I think it's fine to simply identify the problem and indicate that, at this point at the input, we have skipped something that could be important.
| } | ||
| outerMostPanel.add(editor, EDITOR_SPOT_COMPONENT); | ||
| } | ||
| editorSplitter.setFirstComponent(editor); |
There was a problem hiding this comment.
I love the simplification of this code!
There was a problem hiding this comment.
Right, that was a pleasant surprise!
There was a problem hiding this comment.
I just noticed a problem though: the bottom console grows automatically with each new line of text that is added inside. That's not really desired
There was a problem hiding this comment.
Is it possible to set a maximum height?
|
hi @bjansen I added a few in-line comments. Looking great! |
|
Should I wait for this before pushing 1.19? |
This is a proposed fix for #523