Re: [heycam/webidl] Refactor integer conversion in ES bindings. (#235)

domenic requested changes on this pull request.

Lots of style nits but overall looking great.

> @@ -6507,6 +6510,78 @@ when passed to a [=platform object=] expecting that type, and how IDL values
 of that type are <dfn id="dfn-convert-idl-to-ecmascript-value" export lt="converted to an ECMAScript value|converted to ECMAScript values">converted to ECMAScript values</dfn>
 when returned from a platform object.
 
+<div algorithm>
+
+    <dfn lt="AssertFinite" abstract-op>AssertFinite(|v|)</dfn>:
+
+    1.  Let |r| be [=!=][=isFinite=](|v|).

You can't use ES functions as if they are abstract operations. Better to just inline it.

> @@ -6507,6 +6510,78 @@ when passed to a [=platform object=] expecting that type, and how IDL values
 of that type are <dfn id="dfn-convert-idl-to-ecmascript-value" export lt="converted to an ECMAScript value|converted to ECMAScript values">converted to ECMAScript values</dfn>
 when returned from a platform object.
 
+<div algorithm>
+
+    <dfn lt="AssertFinite" abstract-op>AssertFinite(|v|)</dfn>:

"Assert" in spec land generally means "this can never happen" not "throw an error if this happens". Maybe "EnsureFinite" or "RequireFinite" instead.

> @@ -6507,6 +6510,78 @@ when passed to a [=platform object=] expecting that type, and how IDL values
 of that type are <dfn id="dfn-convert-idl-to-ecmascript-value" export lt="converted to an ECMAScript value|converted to ECMAScript values">converted to ECMAScript values</dfn>
 when returned from a platform object.
 
+<div algorithm>
+
+    <dfn lt="AssertFinite" abstract-op>AssertFinite(|v|)</dfn>:
+
+    1.  Let |r| be [=!=][=isFinite=](|v|).
+    1.  If |r| is <emu-val>false</emu-val>,
+        then <a lt="es throw">throw a <emu-val>TypeError</emu-val></a>.
+</div>
+
+<div algorithm>
+
+    <dfn lt="IntegerPart" abstract-op>IntegerPart(|n|)</dfn>:
+
+    1.  Let |r| be [=floor=]([=abs=](|n|))
+    1.  If |n| &lt; 0, then return -1 * |r|.

Use × not *

> +        1.  If |type| is "unsigned", then let |lowerBound| be 0.
+        1.  Otherwise let |lowerBound| be −2<sup>53</sup>.
+
+            Note: in ECMAScript, all numbers including integers are represented as
+            double-precision 64 bit IEEE 754 floating point numbers.
+
+            Issue: Complete this explanation once I understand why we're only concerned
+            about this when clamping and not for all values.
+
+    1.  Otherwise, if |type| is "unsigned", then:
+        1.  Let |lowerBound| be 0.
+        1.  Let |upperBound| be 2<sup>|bitLength|</sup> - 1.
+    1.  Otherwise:
+        1.  Let |lowerBound| be -2<sup>|bitLength - 1|</sup>.
+        1.  Let |upperBound| be 2<sup>|bitLength - 1|</sup> - 1.
+    1.  Initialize |x| to [=ToNumber=](|V|).

Let x be ? ToNumber(V)

> +
+    1.  Otherwise, if |type| is "unsigned", then:
+        1.  Let |lowerBound| be 0.
+        1.  Let |upperBound| be 2<sup>|bitLength|</sup> - 1.
+    1.  Otherwise:
+        1.  Let |lowerBound| be -2<sup>|bitLength - 1|</sup>.
+        1.  Let |upperBound| be 2<sup>|bitLength - 1|</sup> - 1.
+    1.  Initialize |x| to [=ToNumber=](|V|).
+    1.  If the conversion to an IDL value is being performed due to any of the following:
+        *   |V| is being assigned to an [=attribute=] annotated with the [{{EnforceRange}}] [=extended attribute=],
+        *   |V| is being passed as an [=operation=] argument annotated with the [{{EnforceRange}}] extended attribute, or
+        *   |V| is being used as the value of a [=dictionary member=] annotated with the [{{EnforceRange}}] extended attribute,
+
+        then:
+
+        1.  Perform [=?=]<a abstract-op>AssertFinite</a>(|x|).

Space between ?/! and the abstract op

> +
+    1.  If |x| is not <emu-val>NaN</emu-val> and the conversion to an IDL value is being performed due to any of the following:
+        *   |V| is being assigned to an [=attribute=] annotated with the [{{Clamp}}] [=extended attribute=],
+        *   |V| is being passed as an [=operation=] argument annotated with the [{{Clamp}}] extended attribute, or
+        *   |V| is being used as the value of a [=dictionary member=] annotated with the [{{Clamp}}] extended attribute,
+
+        then:
+
+        1.  Set |x| to [=min=]([=max=](|x|, |lowerBound|), |upperBound|).
+        1.  Round |x| to the nearest integer, choosing the even integer if it lies halfway between two,
+            and choosing +0 rather than −0.
+        1.  Return |x|.
+    1.  If |x| is <emu-val>NaN</emu-val>, +0, −0, +∞, or −∞,
+        then return 0.
+    1.  Set |x| to [=!=]<a abstract-op>IntegerPart</a>(|x|).
+    1.  Set |x| to |x| [=modulo=](2<sup>|bitLength|</sup>).

x modulo 2^bitLength, not x modulo(2^bitLength)

> +    1.  Let |r| be [=!=][=isFinite=](|v|).
+    1.  If |r| is <emu-val>false</emu-val>,
+        then <a lt="es throw">throw a <emu-val>TypeError</emu-val></a>.
+</div>
+
+<div algorithm>
+
+    <dfn lt="IntegerPart" abstract-op>IntegerPart(|n|)</dfn>:
+
+    1.  Let |r| be [=floor=]([=abs=](|n|))
+    1.  If |n| &lt; 0, then return -1 * |r|.
+    1.  Otherwise return |r|.
+</div>
+
+<div algorithm>
+    <dfn lt="ConvertToInt" abstract-op>ConvertToInt(|V|, |bitLength|, |type|)</dfn>:

Maybe type should be "signedness"?

> +    1.  If |x| is not <emu-val>NaN</emu-val> and the conversion to an IDL value is being performed due to any of the following:
+        *   |V| is being assigned to an [=attribute=] annotated with the [{{Clamp}}] [=extended attribute=],
+        *   |V| is being passed as an [=operation=] argument annotated with the [{{Clamp}}] extended attribute, or
+        *   |V| is being used as the value of a [=dictionary member=] annotated with the [{{Clamp}}] extended attribute,
+
+        then:
+
+        1.  Set |x| to [=min=]([=max=](|x|, |lowerBound|), |upperBound|).
+        1.  Round |x| to the nearest integer, choosing the even integer if it lies halfway between two,
+            and choosing +0 rather than −0.
+        1.  Return |x|.
+    1.  If |x| is <emu-val>NaN</emu-val>, +0, −0, +∞, or −∞,
+        then return 0.
+    1.  Set |x| to [=!=]<a abstract-op>IntegerPart</a>(|x|).
+    1.  Set |x| to |x| [=modulo=](2<sup>|bitLength|</sup>).
+    1.  If |type| is "unsigned" and |x| ≥ 2<sup>|bitLength| - 1</sup>,

Use − (minus sign) instead of - (hyphen-minus) consistently.

> @@ -6507,6 +6510,78 @@ when passed to a [=platform object=] expecting that type, and how IDL values
 of that type are <dfn id="dfn-convert-idl-to-ecmascript-value" export lt="converted to an ECMAScript value|converted to ECMAScript values">converted to ECMAScript values</dfn>
 when returned from a platform object.
 
+<div algorithm>

IMO it would be reasonable to create a subsection for integer types, and give each type a heading under it but also give these supporting abstract ops a heading under it. (Abstract ops after the integer types, I think.)

> @@ -6970,8 +6857,7 @@ may return any value, which will be discarded.
     to an IDL {{float}} value by running the following algorithm:
 
     1.  Let |x| be [=ToNumber=](|V|).

Add ?

> @@ -7043,8 +6929,7 @@ value when its bit pattern is interpreted as an unsigned 32 bit integer.
     to an IDL {{double}} value by running the following algorithm:
 
     1.  Let |x| be [=ToNumber=](|V|).

Add ?

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/heycam/webidl/pull/235#pullrequestreview-8216141