[CSharp] Fix for #4386 -- change signatures for ReportAttemptingFullContext() and ReportContextSensitivity() to be identical to all other targets #4399
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…Sensitivity() to be identical to all other targets Signed-off-by: Ken Domino <[email protected]>
|
lgtm |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is a fix for #4386: fix signatures of ReportAttemptingFullContext() and ReportContextSensitivity() for the CSharp.
In this PR, I changed the signatures for ReportAttemptingFullContext() and ReportContextSensitivity() for the CSharp runtime to be identical to that for all the other runtimes. The code before this change appears to be written with the idea of exposing the prediction context in some other way. But, it seems the idea never was thought completely out, and what was implemented was basically useless null pointers being passed around for no reason. This change exposes the ATN config sets so I can then actually create meaningful error messages and perform a better analysis of why a grammar is performing poorly. Without the change, we are left with code that doesn't work the same way as all the other targets, and doesn't really help us to figure out why a grammar is bad.
Incidentally, you may be wondering why ReportAmbiguity() does not have the same problem with the signature as the others. It appears that came from initial check-in of the runtime for CSharp. e8c4bc4#diff-46a0f5d3e67ea8c3d1c4fe92bd6fbbe9bcdfcf01fd05b6cb8bcc4286ac532488R99
There are no tests for this API change. In fact, there were no tests for any of the API to display diagnostic errors for CSharp. And, it's unfair to require I write all these tests from scratch at this point. It should have been done years ago, with this a commit with just one word for the description.