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

remove error branch from LUT if it is unreachable #386

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

RustyYato
Copy link
Contributor

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

@jeertmans
Copy link
Collaborator

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 :-)

@jeertmans jeertmans added the enhancement New feature or request label Apr 9, 2024
@RustyYato
Copy link
Contributor Author

Would you like a test like in logos-cli? I think I could extend that to check the codegen of the LUT, or did you have something else in mind?

@RustyYato
Copy link
Contributor Author

friendly ping @jeertmans 😄

@jeertmans
Copy link
Collaborator

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 :)

@RustyYato
Copy link
Contributor Author

Hey, I will be out for the next few days, but I'll get back to this once I'm back. 😀

@RustyYato
Copy link
Contributor Author

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.

@RustyYato
Copy link
Contributor Author

I've just updated the tests in logos-cli to handle multiple inputs at once, so it becomes easier to add new codegen tests in the future.
LMK if you had something else in mind/

@jeertmans
Copy link
Collaborator

Looks great, thanks @RustyYato!

I have 2 remarks / suggestions:

  • I would rather put those tests in the tests folder, as they are more "integration" tests than logos-cli tests;
  • I think using rstest's fixture might be more readable. You could also build a PathBuf using join methods instead and unwrap it, so we don't use OS-specific separators (I know it works because they will translate / to \\ On Windows, but still).

Don't hesitate to give your opinion on this :-)

@RustyYato
Copy link
Contributor Author

RustyYato commented May 6, 2024

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 reverted my original tests commit, and added a new test function which is parameterized on the codegen directory name, and added an env var BLESS_CODEGEN to update the codegen tests if they need to be updated all at once.
How does that look?

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Unreachable branch in LUTs are still linked to
2 participants