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

Move all matchers to strict #493

Open
rengolin opened this issue Apr 20, 2023 · 2 comments
Open

Move all matchers to strict #493

rengolin opened this issue Apr 20, 2023 · 2 comments
Labels
stability Improves stability and allows forward progress

Comments

@rengolin
Copy link
Contributor

As exposed in #492, TPP matchers all have asserts to make sure the number of ops is correct. This is a problem because:

  1. It is poor software engineering practice to have to check the validity of functions. Checks should be in the functions.
  2. If the number of arguments is wrong, the match should return false in the first place.
  3. We don't want to "match-then-bail" on cases where the pattern does not match. We want to leave the code alone.

As standard compiler assumptions we must:

  • Assume the most strict matches possible, transform what we can.
  • Extend what strict means with time, to only the cases we know are safe and correct.

This leads to false negatives, as usual, but it gets rid of false positives. The former is a missed opportunity, the latter, can lead to broken code generation.

@adam-smnk @chelini

PS: We need to search for that pattern elsewhere and fix when it breaks the same assumption as this issue.

@rengolin rengolin added the stability Improves stability and allows forward progress label Apr 20, 2023
@chelini
Copy link
Contributor

chelini commented Apr 20, 2023

Are you proposing to remove the asserts?

@rengolin
Copy link
Contributor Author

Are you proposing to remove the asserts?

Any assert that validates the return of a matcher, yes. If the matcher returns true, then it's a match and we shouldn't have to second-guess. If we have an assert, we should move it inside the matcher and return false instead.

Other asserts are perfectly fine.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
stability Improves stability and allows forward progress
Projects
None yet
Development

No branches or pull requests

2 participants