Re: [heycam/webidl] Allow extended attributes to apply to types (#286)

tobie approved this pull request.

Introducing the annotated type and annotated inner type concepts clarifies everything imho. I like it.

LGTM pending the few nits.

> @@ -3343,7 +3329,7 @@ the following algorithm returns <i>true</i>.
     [=member type=] of the union type is distinguishable
     with the non-union type,
     or <i>false</i> otherwise.
-1.  If the two types (taking their [=inner types=]
+1.  If the two types (taking their [=nullable types/inner type=]

Nice catch on the incorrect original plural here.

>  
     1.  Let |T| be the [=union type=].
     1.  Initialize |S| to ∅.
     1.  For each [=member type=] |U| of |T|:
         1.  If |U| is a [=nullable type=], then
-            set |U| to be the [=inner type=] of |U|.
+            set |U| to be the [=nullable types/inner type=] of |U|.
+        1.  If |U| is an [=annotated type=], then
+            set |U| to be the [=annotated types/inner type=] of |U|.

Shouldn't these two steps' order be reversed to catch the inner type of cases such as: `[foo] BarType?`?



> +<div class="example">
+    <code>[Clamp] long</code> defines a new [=annotated type=], whose behavior is based on that of
+    the [=annotated types/inner type=] {{long}}, but modified as specified by the [{{Clamp}}]
+    extended attribute.
+</div>
+
+The following extended attributes are <dfn for="extended attributes">applicable to types</dfn>:
+[{{Clamp}}],
+[{{EnforceRange}}], and
+[{{TreatNullAs}}].
+
+<div algorithm>
+    The <dfn for="IDL type" lt="extended attribute associated with|extended attributes associated with">extended attributes associated with</dfn>
+    a [=type=] |type| are determined as follows:
+
+    1.  Let |extended attributes| be a new empty [=set=].

I think this should be [=ordered set|set=], as [=set=] links to the ES `Set` abstract operation.

> +        within an <emu-nt><a href="#prod-DictionaryMember">DictionaryMember</a></emu-nt> production,
+        [=set/append=] to |extended attributes| all of the [=extended attributes=] present in the
+        production's
+        <emu-nt><a href="#prod-ExtendedAttributeList">ExtendedAttributeList</a></emu-nt> that are
+        [=applicable to types=].
+    1.  If |type| is a [=typedef=], [=set/append=] the [=extended attributes associated with=] the
+        [=type being given a new name=] to |extended attributes|.
+    1. Return |extended attributes|.
+</div>
+
+For any [=type=], the [=extended attributes associated with=] it must only contain
+[=extended attributes=] that are [=applicable to types=].
+
+The [=type name=] of a type associated with [=extended attributes=] is the concatenation of the
+type name of the original type with the set of strings corresponding to the [=identifiers=] of each
+[=extended attribute associated with=] the type, in order.

So upon reading this again, it's quite clear my previous comment on the concatenation ordering wasn't well thought out.

My concern here was to make sure that: `[Foo, Bar] Type` and `[Bar, Foo] Type` didn't create two different types.

So maybe this should read something like: 

```
[=extended attribute associated with=] the type, in lexicographic order.
```

-- 
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/286#pullrequestreview-29700063