Add [EnforceRange] to all integers #498

kainino0x · 2019-11-14T17:25:22Z

[EnforceRange] is the most-strict and least-surprising mode of number-to-integer conversion in WebIDL. (The default is modular, the other option is [Clamp].) [EnforceRange] rounds the (double-precision floating point) number to an integer, then throws if it's outside of the range of the integer type. It has the benefit that removing this attribute would be a non-breaking change (in that it would change from throwing to not-throwing).

I've been meaning to do this for a long time, but I didn't realize that it was an attribute on the type and not the fields/parameters themselves, or I might have done this before removing all the typedefs. 🙃

I'm not attached to using typedefs, it just makes it slightly more apparent if something diverges from this (IMO) (the word long will appear). This PR does not leave any unenforced integer types.

Austin did some quick measurements in Chrome's bindings, and this doesn't seem to impact performance in the non-failure case, for us.

Preview | Diff

JusSn · 2019-11-18T19:36:09Z

This change, I like it. Another!

kdashg

I don't like the verbose name. I might even prefer [EnforceRange] u32. Is there even a use for non-enforced vars?

kvark

Ideally, I'd prefer us to have semantic typedefs for all the enforced things, like we have for buffer addresses today - GPUBufferSize.

spec/index.bs

kainino0x · 2019-11-19T18:22:39Z

I'm out, so please feel free to bikeshed this while I'm out. :)

…

On Mon, Nov 18, 2019, 10:10 PM Dzmitry Malyshau ***@***.***> wrote: ***@***.**** approved this pull request. Ideally, I'd prefer us to have semantic typedefs for all the enforced things, like we have for buffer addresses today - GPUBufferSize. ------------------------------ In spec/index.bs <#498 (comment)>: > @@ -158,7 +158,10 @@ Type Definitions {#type-definitions} ============================ <script type=idl> -typedef unsigned long long GPUBufferSize; +typedef [EnforceRange] unsigned long long GPUBufferSize; +typedef [EnforceRange] unsigned long long enforced_u64; shouldn't this be camel case, like EnforcedU32? — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#498?email_source=notifications&email_token=AAEUBEZKTDE747HL4CXQVY3QUMADTA5CNFSM4JNPAMK2YY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCL66WTY#pullrequestreview-318630735>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAEUBE4RTMHCWEB4LCGSD2TQUMADTANCNFSM4JNPAMKQ> .

Kangz · 2019-11-25T20:59:17Z

Discussed and approved the semantics in the 2019-11-25 meeting. Needs bikeshedding on the name.

kainino0x

I've revised this PR. Please review!

kainino0x · 2019-12-03T01:41:42Z

spec/index.bs

+typedef [EnforceRange] unsigned long GPUIntegerCoordinate;
+typedef [EnforceRange] unsigned long GPUIndex32;
+typedef [EnforceRange] unsigned long GPUSize32;
+typedef [EnforceRange] long GPUSignedOffset32;


Note: I tried not to go too overboard with the number of different types, so consolidated some of them into these generic ones.

kainino0x · 2019-12-03T01:43:57Z

Is there even a use for non-enforced vars?

IMO, no.

kdashg

I would rather see u32_strict (U32Strict? Uint32Strict?) or similar, rather than hiding the sizes behind semantic types.

Kangz · 2020-01-06T20:48:27Z

This was discussed in the 2020-01-06 meeting

kainino0x · 2020-01-27T22:14:15Z

I'd like to get some resolution from @kvark @jdashg so we can land this.

kvark

kdashg · 2020-02-11T01:19:36Z

Approved, but has rebase conflicts. @kainino0x

- Rename GPUBufferSize to GPUSize64 (it's also used for ArrayBuffer). - Use GPUSize32 instead of GPUIntegerCoordinate for rowPitch/imageHeight. (They have the same definition though.)

kainino0x · 2020-02-11T03:08:58Z

Merging after merging with master, and with minor nonfunctional changes (see the last commit)

Adds [EnforceRange] to most of the integer types in WebGPU according to my spec change: gpuweb/gpuweb#498 . Note that there is one place where it was not possible to match that spec change, as that the spec change violates WebIDL (in spirit). - Interface `const` values for flags can't use [EnforceRange]'d types. The upstream bug for that is gpuweb/gpuweb#549 There is a manual workaround in cts.html for an incompatibility between the variant strings and run_web_tests:  Bug: 1050909 Change-Id: Ic84f102d138204328fe0d4fb7cb46622c7104630 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2048998 Auto-Submit: Kai Ninomiya <kainino@chromium.org> Commit-Queue: Yutaka Hirano <yhirano@chromium.org> Reviewed-by: Yutaka Hirano <yhirano@chromium.org> Reviewed-by: Austin Eng <enga@chromium.org> Cr-Commit-Position: refs/heads/master@{#762254}

* Add [EnforceRange] to all integers * more integer types * use GPUSize32 for first* * Changes to naming and which types are used - Rename GPUBufferSize to GPUSize64 (it's also used for ArrayBuffer). - Use GPUSize32 instead of GPUIntegerCoordinate for rowPitch/imageHeight. (They have the same definition though.)

* Implement api,operation,rendering,draw:arguments:* * Address comments from code review * Update src/webgpu/api/operation/rendering/draw.spec.ts

kdashg reviewed Nov 18, 2019

View reviewed changes

kvark approved these changes Nov 18, 2019

View reviewed changes

spec/index.bs Outdated Show resolved Hide resolved

kainino0x added 2 commits December 2, 2019 16:54

Add [EnforceRange] to all integers

c5f8cee

more integer types

efe292c

kainino0x force-pushed the enforcerange branch from b88370c to efe292c Compare December 3, 2019 01:40

kainino0x commented Dec 3, 2019

View reviewed changes

use GPUSize32 for first*

b0d2776

kainino0x requested review from kdashg and kvark December 4, 2019 03:56

kvark mentioned this pull request Dec 4, 2019

GPUQueue.writeBuffer/writeTexture #509

Closed

kdashg reviewed Dec 4, 2019

View reviewed changes

kdashg approved these changes Jan 28, 2020

View reviewed changes

kvark approved these changes Feb 10, 2020

View reviewed changes

Merge branch 'master' into enforcerange

3135644

kainino0x force-pushed the enforcerange branch from e5a877f to 7fb3149 Compare February 11, 2020 03:07

Changes to naming and which types are used

75f212b

- Rename GPUBufferSize to GPUSize64 (it's also used for ArrayBuffer). - Use GPUSize32 instead of GPUIntegerCoordinate for rowPitch/imageHeight. (They have the same definition though.)

kainino0x force-pushed the enforcerange branch from 7fb3149 to 75f212b Compare February 11, 2020 03:07

kainino0x merged commit 56b4d3b into gpuweb:master Feb 11, 2020

kainino0x deleted the enforcerange branch February 11, 2020 03:13

This was referenced Feb 11, 2020

Switch dynamicOffsetsDataLength to 32 instead of 64 #548

Merged

Match EnforceRange rules from WebIDL #549

Closed

Add [EnforceRange] to all integers #498

Add [EnforceRange] to all integers #498

Uh oh!

Conversation

kainino0x commented Nov 14, 2019 • edited by pr-preview bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

JusSn commented Nov 18, 2019

Uh oh!

kdashg left a comment

Choose a reason for hiding this comment

Uh oh!

kvark left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

kainino0x commented Nov 19, 2019 via email

Uh oh!

Kangz commented Nov 25, 2019

Uh oh!

kainino0x left a comment

Choose a reason for hiding this comment

Uh oh!

kainino0x Dec 3, 2019

Choose a reason for hiding this comment

Uh oh!

kainino0x commented Dec 3, 2019

Uh oh!

kdashg left a comment

Choose a reason for hiding this comment

Uh oh!

Kangz commented Jan 6, 2020

Uh oh!

kainino0x commented Jan 27, 2020

Uh oh!

kvark left a comment

Choose a reason for hiding this comment

Uh oh!

kdashg commented Feb 11, 2020

Uh oh!

kainino0x commented Feb 11, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

kainino0x commented Nov 14, 2019 •

edited by pr-preview bot

Loading