-
Notifications
You must be signed in to change notification settings - Fork 345
No explicit grammar for decorations #689
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
Conversation
876aa64 to
151565f
Compare
|
Discussed at the WGSL 2020-07-21 meeting with the resolution to accept this change once it's been updated. |
c193dcb to
8951c8d
Compare
kvark
left a comment
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.
I see two concerns that you are trying to address here. Please correct if this is wrong.
- Free the implementations from the need to validate the same things twice.
As we discussed on the call, no (currently developed) implementations are going to change because of this PR. There is no practical aspect of this for implementations.
- Reduce the complexity of the grammar, since it can't enforce the full range of validation rules.
Of course full validation can't happen at the grammar level. I think that's obvious.
So where do we draw the line between stuff that needs to be in the grammar then?
For example, the grammar currently has storage_class. Clearly, not all storage classes are expected in all contexts. Does it mean we should be throwing it away and replacing it with IDENT?
My understanding is that the grammar helps with "first stage" validation. I.e. the validator would work with "here is a list of appropriate decorations for this element" instead of "here is a list of key-value strings".
|
Sorry for raising these concerns over the PR, I now noticed that it's marked as resolved. I haven't put enough thought into it previously when we were discussing this, but feel free to proceed if the concerns are not ringing any bells :) |
kvark
left a comment
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.
Talked to @jdashg offline and got convinced that this is fiine :)
|
Can this be landed once it's sync'd? |
|
Discussed at the 2020-09-29 meeting. @jdashg will address comments |
Decorations can't be validated much at parse time, so they shouldn't be tightly specified in the grammar. "[location(0), builtin(0)]" =parsing=> "Error: Failed to parse token after builtin, expected IDENT." "[location(0), builtin(frag_coord)]" =parsing=> OK =compilation=> "Error: frag_coord cannot be an output of a vertex entrypoint." Since having it in the grammar doesn't really reduce the validation load on the compiler, we might as well not constrain it in the grammar. (even if this double-validation is cheap/free)
68d39c9 to
2fc6606
Compare
* Add refttests for different size of canvas and swapchain This tests covers the cases that user config webgpu back buffer with larger than/smaller than/equal to canvas size. And the content should be auto scale to the canvas size and keep the content. WebGPU canvas should present things by doing downscaling/upscaling/no-changes for the back buffer. * Using CSS to have a correct ref * Address comments * add one pixel to make sure image isn't downsampled to canvas element resolution Co-authored-by: Kai Ninomiya <kainino@chromium.org>
Split into two parts, off of #655. This will need to be rebased after #655.
Decorations can't be validated much at parse time, so they shouldn't be tightly specified in the grammar.
"[location 0, builtin 0]" =parsing=> "Error: Failed to parse token after
builtin, expected IDENT.""[location 0, builtin frag_coord]" =parsing=> OK =compilation=> "Error:
frag_coordcannot be an output of a vertex entrypoint."Since having it in the grammar doesn't really reduce the validation load on the compiler, we might as well not constrain it in the grammar. (even if this double-validation is cheap/free)