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
remove error branch from LUT if it is unreachable #386
base: master
Are you sure you want to change the base?
Conversation
Hello @RustyYato! Thank you for your PR! Can you implement a small test that generates the LUT and checks that, indeed, the error branch is removed if it is unreachable? Tell me if you need help with setting up the test :-) |
Would you like a test like in |
friendly ping @jeertmans 😄 |
Ooos sorry @RustyYato! yes the easiest way to test this might be to test against the output of the logos-cli. So you can generate a very basic scenario: one lexer that has a useful error branch, and one whose error branch was removed :) To check that your tests work, you can remove your patch and first check that they fail, then apply your patch and check they now work :) |
Hey, I will be out for the next few days, but I'll get back to this once I'm back. 😀 |
Hmm, it looks like the lookup table isn't taken in some cases (specifically the one in the related issue). So there may be some other low hanging fruit to remove error cases. But I'll add a test for one that does use the LUT and solve the other cases separately. |
I've just updated the tests in |
Looks great, thanks @RustyYato! I have 2 remarks / suggestions:
Don't hesitate to give your opinion on this :-) |
This reverts commit 0dfcf00.
Ok, in that case I think it would be nice to make it easy to add new tests and update old tests if codegen changes. I can rebase the changes right before merging to clean up the history if you would like. I find rebases during reviews make reviewing harder so didn't do that yet. |
Fixes #385
This removes unreachable branches from LUTs in the generated code, so that LLVM can see that the error case is unreachable for lexers like in the linked issue