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

k:.Idx n doesn't make sense #106

Closed
leissa opened this issue Oct 12, 2022 · 2 comments · Fixed by #112
Closed

k:.Idx n doesn't make sense #106

leissa opened this issue Oct 12, 2022 · 2 comments · Fixed by #112
Labels
bug Something isn't working

Comments

@leissa
Copy link
Member

leissa commented Oct 12, 2022

My latest commit fixed some UB which materialized in the CI that this test here

ds2cps_ax_cps2ds_dependent2.thorin

failed to work sometimes on Windows.

Problem is: A literal 42:.Idx n doesn't make sense and should have been a type error (which it is now). It caused UB in some normalizers (which I have fixed).

One edge case is: 0:.Idx n. This kind of makes sense, as 0:.Idx 0 actually means 0:.Idx 2^64. So this literal is guaranteed to exist - but this is still kind of odd Oo Should we really allow that?

For now I disabled the test case above, as it makes use of this erroneous index.

See also #101, #105.

tl;dr:

  • We need to fix ds2cps_ax_cps2ds_dependent2.thorin
  • should we allow 0:.Idx n?
@leissa leissa added the bug Something isn't working label Oct 12, 2022
@NeuralCoder3
Copy link
Collaborator

A cast s2s/u2u should also fix the problem is probably the correct fix.

I think we should allow 0:.Idx n as it might be helpful to have a zero sometimes and it exists from a theoretical point of view.

@leissa
Copy link
Member Author

leissa commented Oct 13, 2022

@NeuralCoder3: can you fix the ds2cps_ax_cps2ds_dependent2.thorin test case? Then, I can close this one here already.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants