Some bugfixes #3564
Some bugfixes #3564
Conversation
| g.tool.errMgr.toolError(ErrorType.INTERNAL_ERROR, "element list has first|last == null"); | ||
| } | ||
| return new Handle(first.left, last.right); | ||
| Handle last = els.get(n - 1); |
There was a problem hiding this comment.
That's a significant change in behavior, that might break users code
There was a problem hiding this comment.
I've just removed first==null || last==null check, null elements are being processed later. How it could affect user code? I don't understand why it's a significant change in behavior. All tests are passing and there is no test on INTERNAL_ERROR (except for the first test added in this PR).
There was a problem hiding this comment.
It no longer throws a tool error, that's a significant change.
Not sure how to reproduce the throwing scenario, but it would be better to have a test for that (which throws with the existing code) and check that it still does with the new code.
We never know what developers do with development tools, they might be sending some random grammars and expecting an error if the grammar isn't valid...
There was a problem hiding this comment.
My test covers NPE from definite issue #2788. If I restore the check, two errors will occur:
error(20): Test.g4:2:30: internal error: Rule error undefined
error(20): internal error: element list has first|last == null
The message internal error: element list has first|last == null is duplicated, completely unclear for the end-user, and does not help to resolve an issue. The first message is much more informative.
We never know what developers do with development tools, they might be sending some random grammars and expecting an error if the grammar isn't valid...
I'm almost sure there are unfixed errors in the tool but I think real issues are more important than potential ones in random grammars. We can fix them later if they occur.
There was a problem hiding this comment.
I'm not saying this is important, but not sure the first message can't be improved ? How about putting the undefined rules in quotes ?
There was a problem hiding this comment.
Maybe it can, but I didn't change it. This is the existing error that is being thrown in https://github.com/antlr/antlr4/blob/dev/tool/src/org/antlr/v4/automata/ParserATNFactory.java#L297 Also, it looks like we don't use quotes for other similar errors, see for example
|
Hi. I think I will close this because as I mentioned I'm not a big fan of removing IFs in favor of |
Actually I didn't replace I can rollback code style changes. But I don't see significant changes of ATN factory, I've fixed NRE that should not be thrown, it's quite important especially for plugin. |
|
Hi. please roll back to avoid style changes so that I can see the actual changes. :) then I can take a quickly look :) |
|
It's already done :) #3632 |
No description provided.