IDL: overload resolution for string and enumeration arguments

Source/bindings/scripts/v8_interface.py

Index: Source/bindings/scripts/v8_interface.py
diff --git a/Source/bindings/scripts/v8_interface.py b/Source/bindings/scripts/v8_interface.py
index 4541f11457fdaec287be5e17695651ae900e0ac5..f5cd7ed77451378ee6c02a5c0fc2fb228713b3e9 100644
--- a/Source/bindings/scripts/v8_interface.py
+++ b/Source/bindings/scripts/v8_interface.py
@@ -380,15 +380,14 @@ def is_use_spec_algorithm(effective_overloads_by_length):
     # Use spec algorithm if:
     # * method is not variadic,
     # * no distinguishing type is unsupported:
-    #   non-wrapper, array, callback interface, enumeration, boolean, string, nullable,
+    #   non-wrapper, array, callback interface, boolean, nullable,
     # * all distinguishing types are distinct, and
     # * all distinguishing arguments are required (not optional).
     def is_unsupported_type(idl_type):
         return ((idl_type.is_interface_type and not idl_type.is_wrapper_type) or
                 idl_type.array_or_sequence_type or
                 idl_type.is_callback_interface or
-                idl_type.is_enum or
-                idl_type.name in ('Boolean', 'String') or
+                idl_type.name == 'Boolean' or
                 idl_type.is_nullable)
 
     for _, effective_overloads in effective_overloads_by_length:
@@ -636,37 +635,66 @@ def resolution_tests_methods(effective_overloads):
     idl_types = [argument['idl_type_object'] for argument in arguments]
     idl_types_methods = zip(idl_types, methods)
 
-    # We can't do a single loop through all methods or simply sort them, because
+    # We can’t do a single loop through all methods or simply sort them, because
     # a method may be listed in multiple steps of the resolution algorithm, and
     # which test to apply differs depending on the step.
-    # Instead, we need to loop through all methods at each step, filtering to
-    # matches and generating an appropriate test.
-    for idl_type, method in idl_types_methods:
-        # 4. Otherwise: if V is a platform object – but not a platform array
-        # object – and there is an entry in S that has one of the following
-        # types at position i of its type list,
-        # • an interface type that V implements
-        # (We distinguish wrapper types from built-in interface types.)
-        if idl_type.is_wrapper_type:
-            test = 'V8{idl_type}::hasInstance({cpp_value}, info.GetIsolate())'.format(idl_type=idl_type.base_type, cpp_value=cpp_value)
+    #
+    # Instead, we need to go through all methods at each step, either finding
+    # first match (if only one test is allowed) or filtering to matches (if
+    # multiple tests are allowed), and generating an appropriate tests.
+
+    # 4. Otherwise: if V is a platform object – but not a platform array
+    # object – and there is an entry in S that has one of the following
+    # types at position i of its type list,
+    # • an interface type that V implements
+    # (We distinguish wrapper types from built-in interface types.)
+    for idl_type, method in ((idl_type, method)
+                             for idl_type, method in idl_types_methods
+                             if idl_type.is_wrapper_type):
+        test = 'V8{idl_type}::hasInstance({cpp_value}, info.GetIsolate())'.format(idl_type=idl_type.base_type, cpp_value=cpp_value)
+        yield test, method

[haraken, 2014/05/14 06:09:03]

Why do you not need a try-catch block here?

[Nils Barth (inactive), 2014/05/14 06:28:15]

Because this can return multiple matches.
Added comment to clarify.

+
+    # (Check for exact type matches before performing automatic type conversion;
+    # only needed if distinguishing between primitive types.)
+
+    # 10. Otherwise: if V is a Number value, and there is an entry in S that has
+    # one of the following types at position i of its type list,
+    # • a numeric type
+    if any(idl_type.name == 'String' or idl_type.is_enum

[haraken, 2014/05/14 06:09:03]

I wonder why you need to check string or enum here.

[Nils Barth (inactive), 2014/05/14 06:28:15]

It's only needed if there's a match in step 11, otherwise it's redundant.
Added comment to address this.

+           for idl_type in idl_types):
+        try:
+            method = next(method for idl_type, method in idl_types_methods
+                          if idl_type.is_numeric_type)
+            test = '%s->IsNumber()' % cpp_value
             yield test, method
+        except StopIteration:
+            pass
+
+    # (Perform automatic type conversion, in order. If any of these match,
+    # that’s the end, and no other tests are needed.)
 
-    # FIXME: not needed yet
-    # for idl_type, method in idl_types_methods:
-    #     # 10. Otherwise: if V is a Number value, and there is an entry in S
-    #     # that has one of the following types at position i of its type
-    #     # list,
-    #     # • a numeric type
-    #     if idl_type.is_numeric_type:
-    #         test = '%s->IsNumber()' % cpp_value
-    #         yield test, method
-
-    for idl_type, method in idl_types_methods:
-        # 12. Otherwise: if there is an entry in S that has one of the
-        # following types at position i of its type list,
-        # • a numeric type
-        if idl_type.is_numeric_type:
-            yield 'true', method
+    # 11. Otherwise: if there is an entry in S that has one of the following
+    # types at position i of its type list,
+    # • DOMString
+    # • an enumeration type
+    try:

[Jens Widell, 2014/05/14 05:19:31]

You could skip the 'try' and "move" the controlled block into the preceding
'if', which checks for the same condition. That would avoid checking for a
string-or-enum argument twice using two different types of checks.

[Nils Barth (inactive), 2014/05/14 06:28:15]

While that seems simpler, I'm following the order in the spec;
the added |if| check above is to remove redundancy in the CG.
It gets trickier when we also have booleans.

+        method = next(method for idl_type, method in idl_types_methods
+                      if idl_type.name == 'String' or idl_type.is_enum)
+        yield 'true', method
+        return
+    except StopIteration:
+        pass
+
+    # 12. Otherwise: if there is an entry in S that has one of the following
+    # types at position i of its type list,
+    # • a numeric type

[haraken, 2014/05/14 06:09:03]

I'm curious why the step 11 and 12 are separated. Aren't they doing the same
thing? (just generating "true"). Probably you're planning to make it different
in a follow-up.

[Nils Barth (inactive), 2014/05/14 06:28:15]

Because they're different in the spec, and splitting makes a difference:
it means we *first* resolve to DOMString, *then* to numeric, *regardless*
of the order in the IDL file.
(This is to handle automatic type conversion.)

This is explained in the comment above:
# (Perform automatic type conversion, in order. If any of these match,
# that’s the end, and no other tests are needed.)

+    try:
+        method = next(method for idl_type, method in idl_types_methods
+                      if idl_type.is_numeric_type)
+        yield 'true', method
+        return
+    except StopIteration:
+        pass
 
 
 def overload_resolution_expression(method):