-
Notifications
You must be signed in to change notification settings - Fork 32
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
Emit clear error for index into zero-sized Arrays #171
base: main
Are you sure you want to change the base?
Conversation
Awesome, thanks for the PR! 🍻 I'm not super familiar with this code but the change looks reasonable to me. I'll wait for some other maintainers to look at it before reviewing 🤙 Looks like you need to run |
Somehow this passes locally but not on CI 🤣, I'll have to test a bit more |
I've changed my expectation, previously it was erroring for accessing zero sized arrays, but it was hidden behind a deny that is on by default. Instead, I think we should always panic, but with a clear error message. I'm unsure what the behavior would be if that expression were allowed to be compiled. |
Can we make the error messages match rustc? For example:
|
I can, I do worry that since it's not the actual error it may be more confusing |
Previously GEP on zero-sized arrays, would fail. This change fixes arrays to instead emit runtime arrays. I do not know if this will lead to any runtime cost, but it fixes all the compile errors.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, for the error messages I meant matching the same case in rustc to the case here where it makes sense. I'll leave it up to you to change them so they make sense!
A couple of small comments, thanks again for the PR and sorry for the slow response!
#[spirv(compute(threads(1, 1, 1)))] | ||
pub fn compute() { | ||
let mut array = [0; 0]; | ||
// for some reason this now compiles? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should look into this case, no?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yea, I want to see if it compiles to include a panic, but I'm unsure of how to run the rest to see what happens
Is it run automatically by compiletest
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, sadly compiletest just checks it compiles. We don't have any automated runtime tests right now sadly. I usually just drop the code in one of the examples when I am doing a quick manual test.
Previously GEP on zero-sized arrays, would fail. This change fixes arrays to instead emit runtime arrays. I do not know if this will lead to any runtime cost, but it fixes all the compile errors.
I did try out
[(); 0].map(|()| 1)
, and found that that does not compile.Fixes #151