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

Clarify Valid Values language in the attributes table. #2916

Closed
dj2 opened this issue May 19, 2022 · 5 comments · Fixed by #3143
Closed

Clarify Valid Values language in the attributes table. #2916

dj2 opened this issue May 19, 2022 · 5 comments · Fixed by #3143
Labels
copyediting Pure editorial stuff (copyediting, bikeshed, etc.) wgsl:attributes wgsl WebGPU Shading Language Issues
Milestone

Comments

@dj2
Copy link
Member

dj2 commented May 19, 2022

Currently the Valid Values column says things like integer literal, yielding a positive i32 which I took to mean it could be 1u as an integer literal and the compiler would treat it as i32. This seems to be incorrect and this means it can be AbstractInt or i32 but not u32.

We should make explicit what types can be provided to the various attribute values. Even just making a list like:

  • AbstractInt
  • i32
  • Must be positive

Could be clearer?

@dj2 dj2 added wgsl WebGPU Shading Language Issues copyediting Pure editorial stuff (copyediting, bikeshed, etc.) labels May 19, 2022
@dneto0
Copy link
Contributor

dneto0 commented May 19, 2022

That three-bullet list isn't quite right because its "or" between the first two, and "and" with the last.

Before the last change I had suggested

integer literal evaluating to a positive i32 value

Would that be any clearer?

The 1u cannot evaluate to an i32 value because there is no feasible automatic conversion

@dneto0
Copy link
Contributor

dneto0 commented May 19, 2022

Also, we've deliberately decided to keep the compilers simple by keeping these as literals, not creation-time expressions.
See #2790 (post-V1) and its duplicate #2890

So we definitely want the wording that it must be a literal.

@dj2
Copy link
Member Author

dj2 commented May 19, 2022

Even just:

  • AbstractInt or i32 literal
  • Must be positive

the evaluating could work but spelling out the types is more explicit.

@dneto0
Copy link
Contributor

dneto0 commented May 19, 2022

Works for me.

@Kangz Kangz added this to the V1.0 milestone May 20, 2022
dj2 added a commit to dj2/gpuweb that referenced this issue May 21, 2022
This PR clarifies that the valid values must be either `AbstractInt` or
`i32` instead of the more generate `integer literal`.

Issue gpuweb#2916
dj2 added a commit to dj2/gpuweb that referenced this issue May 21, 2022
This PR clarifies that the valid values must be either `AbstractInt` or
`i32` instead of the more generate `integer literal`.

Issue gpuweb#2916
dj2 added a commit to dj2/gpuweb that referenced this issue May 26, 2022
This PR clarifies that the valid values must be either `AbstractInt` or
`i32` instead of the more generate `integer literal`.

Issue gpuweb#2916
dneto0 pushed a commit that referenced this issue Jun 1, 2022
* [wgsl]: Clarify attribute Valid Values.

This PR clarifies that the valid values must be either `AbstractInt` or
`i32` instead of the more generate `integer literal`.

Issue #2916

* Review feedback
@dneto0
Copy link
Contributor

dneto0 commented Jul 5, 2022

Currently says things like

Must be a positive i32 or AbstractInt literal

It's not clear how far the "or" distributes.

What would be better is to write two sentences: one about types, the other about value.

Must be i32 or AbstractInt
Must be positive.

dneto0 added a commit to dneto0/gpuweb that referenced this issue Jul 5, 2022
Split the consraint into two statements:
 - one is about the tyep
 - other is about the range of values

Fixes: gpuweb#2916
dneto0 added a commit to dneto0/gpuweb that referenced this issue Jul 5, 2022
Split the consraint into two statements:
 - one is about the tyep
 - other is about the range of values

Fixes: gpuweb#2916
dneto0 added a commit that referenced this issue Jul 6, 2022
Split the consraint into two statements:
 - one is about the tyep
 - other is about the range of values

Fixes: #2916
jdarpinian pushed a commit to jdarpinian/gpuweb that referenced this issue Aug 12, 2022
* [wgsl]: Clarify attribute Valid Values.

This PR clarifies that the valid values must be either `AbstractInt` or
`i32` instead of the more generate `integer literal`.

Issue gpuweb#2916

* Review feedback
jdarpinian pushed a commit to jdarpinian/gpuweb that referenced this issue Aug 12, 2022
Split the consraint into two statements:
 - one is about the tyep
 - other is about the range of values

Fixes: gpuweb#2916
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
copyediting Pure editorial stuff (copyediting, bikeshed, etc.) wgsl:attributes wgsl WebGPU Shading Language Issues
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants