-
-
Notifications
You must be signed in to change notification settings - Fork 102
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
chore(lib): remove error branch from LUT if it is unreachable #386
chore(lib): remove error branch from LUT if it is unreachable #386
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. |
friendly ping @jeertmans 🙂 |
Hey @RustyYato, I am very sorry about my delay, was very busy working on my own projects and work, so I put those reviews on hold. Anyway, I think your work is great and deserves to be merged! Thanks for your great work on this! At first, I was waiting because I wanted to think a bit about the best (and cleanest) way to handle those kind tests, but let's merge this and see if we need to improve it (or not) in the future! |
Not a problem, thanks for the reviews! |
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