Closed Bug 749866 Opened 11 years ago Closed 10 years ago

Improve overload resolution to avoid double-testing as much as possible

Categories

(Core :: DOM: Core & HTML, defect)

x86
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla19

People

(Reporter: bzbarsky, Assigned: bzbarsky)

References

(Blocks 1 open bug)

Details

Attachments

(7 files)

Right now overload resolution does a lot of "if this is a Foo object and we have an overload that wants Bar then generate code that unwraps Foo to Bar" kinds of things, but in practice most of those cases in the unwrapping to Bar end up rechecking that we have a Foo object.  A better setup would be to just try unwrapping to Bar and if that fails to move on to the next overload.  That's what we already do for interface types and will do for typed arrays in bug 749864; I think we can also do it for sequences at least.  We just have to make sure our cases are ordered correctly so we comply with the spec.
I should probably wait until Peter finishes up bug 756258, since chances are the same code is involved.
Depends on: 756258
This improvement comes from skipping the extra null-or-undefined codegen for primitives/strings/enums, since we'll end up in their trailing case anyway after skipping all the object types because we're not an object.
This is from the thing the bug was filed on originally: not double-testing the IsArrayLike() bit.
Whiteboard: [need review]
Comment on attachment 672647 [details] [diff] [review]
part 2.  Simplify the code generated by overload resolution a bit when we have sequences or dates at our distinguishing index.

Review of attachment 672647 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/bindings/Codegen.py
@@ +3376,5 @@
> +            # We should not have "any" args at distinguishingIndex,
> +            # since we have multiple possible signatures remaining,
> +            # but "any" is never distinguishable from anything else.
> +            for (_, args) in possibleSignatures:
> +                assert not args[distinguishingIndex].type.isAny()

Why not combine this check and the previous check into the same loop?

@@ +3413,5 @@
> +            # First check for null or undefined.  But ignore nullable
> +            # primitives, strings, and enums here, because if we actually have
> +            # null or undefined it won't unwrap to any of the object arguments
> +            # anyway, so we'll automatically end up in the trailing
> +            # primitive/string/enum case.

A little more explanation (that all object stuff is guarded on isObject, and only one type can be nullable) would be useful here, I think.
Attachment #672647 - Flags: review?(khuey) → review+
> Why not combine this check and the previous check into the same loop?

Done.

> A little more explanation 

Comment now says:

            # First check for null or undefined.  That means looking for
            # nullable arguments at the distinguishing index and outputting a
            # separate branch for them.  But if the nullable argument is a
            # primitive, string, or enum, we don't need to do that.  The reason
            # for that is that at most one argument at the distinguishing index
            # is nullable (since two nullable arguments are not
            # distinguishable), and all the argument types other than
            # primitive/string/enum end up inside isObject() checks.  So if our
            # nullable is a primitive/string/enum it's safe to not output the
            # extra branch: we'll fall through to conversion for those types,
            # which correctly handles null as needed, because isObject() will be
            # false for null and undefined.
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.