Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

golang: Add to quick checks to PredictionContext Equals checks (they … #4571

Merged
merged 1 commit into from Apr 2, 2024

Conversation

ehmm
Copy link
Contributor

@ehmm ehmm commented Apr 1, 2024

…are in use

in other runtimes):

  1. If pointers are equal it's the same object
  2. If we are comparing singletons both types must be singletons

…are in use

in other runtimes):

1. If pointers are equal it's the same object
2. If we are comparing singletons both types must be singletons

Signed-off-by: Eytan Heidingsfeld <32422402+ehmm@users.noreply.github.com>
@jimidle
Copy link
Collaborator

jimidle commented Apr 1, 2024

I would need to do some serious re-checking to accept this PR. I seem to think that I already went down that path and it fails in cases that I do not recall right now.

@ehmm
Copy link
Contributor Author

ehmm commented Apr 1, 2024

Hi @jimidle - I saw some of your previous PRs and the fixes you made, and I understand it is a delicate area - this patch only has 2 additions.

  1. Add a check that if it's the same object itself return true - I don't think there can be a problem with this one as it must be true
  2. If we are comparing a SinglePredictionContext to any other type then return false, I looked at the code (and the Java/C# implementations they both have this check) I can't see where this will provide a false positive, but if you want I can try and take another look or run tests.

We are using antlr for an internal project with quite a complex grammar and I have checked the results after the change and the parse trees are the same, but if you have other tests that you can think of I can run them as well

@parrt
Copy link
Member

parrt commented Apr 1, 2024

Hi all. I'm definitely supportive of improving the runtime, but we definitely have to be careful not to mess up the performance improvements and restructuring @jimidle did. seems that all of the unit tests are passing but maybe @jimidle can try it on some of his super secret internal grammars?

@jimidle
Copy link
Collaborator

jimidle commented Apr 1, 2024

The change does not look incorrect to me and the reasoning is sound. But I definitely want to double check as it is a very change sensitive area that was completely incorrect before the Go changes. I will try to verify this this week.

Also, maybe I could take a look at the grammar and see if we could avoid ambiguities that cause a drop into LL mode.

@parrt
Copy link
Member

parrt commented Apr 1, 2024

Maybe @mike-lischke's fancy new benchmarking system could be applied here to test Go target?

@jimidle
Copy link
Collaborator

jimidle commented Apr 2, 2024

OK - I took a longer look at this and it should not cause any issues. So let's bring this in and I will migrate it to the read-only module for a release shortly.

@parrt parrt merged commit 6628b00 into antlr:dev Apr 2, 2024
41 of 42 checks passed
@parrt
Copy link
Member

parrt commented Apr 2, 2024

Thanks Jim!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants