Bug 172481 - [WebIDL] Overloaded functions unnecessarily duplicate argument checks
Summary: [WebIDL] Overloaded functions unnecessarily duplicate argument checks
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Bindings (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Sam Weinig
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2017-05-22 17:18 PDT by Sam Weinig
Modified: 2017-05-30 20:24 PDT (History)
5 users (show)

See Also:


Attachments
Patch (65.39 KB, patch)
2017-05-23 20:07 PDT, Sam Weinig
cdumez: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Sam Weinig 2017-05-22 17:18:03 PDT
Overloaded functions are currently unnecessarily duplicate argument checks. For example, let's take CanvasRenderingContext2d's stroke function. It has two overloads:

    void stroke(DOMPath path);
    void stroke();

This generates the main bindings function:

EncodedJSValue JSC_HOST_CALL jsCanvasRenderingContext2DPrototypeFunctionStroke(ExecState* state)
{
    VM& vm = state->vm();
    auto throwScope = DECLARE_THROW_SCOPE(vm);
    UNUSED_PARAM(throwScope);
    size_t argsCount = std::min<size_t>(1, state->argumentCount());
    if (argsCount == 0) {
        return jsCanvasRenderingContext2DPrototypeFunctionStroke2(state);
    }
    if (argsCount == 1) {
        return jsCanvasRenderingContext2DPrototypeFunctionStroke1(state);
    }
    return throwVMTypeError(state, throwScope);
}

and then two the helpers, jsCanvasRenderingContext2DPrototypeFunctionStroke1 and jsCanvasRenderingContext2DPrototypeFunctionStroke2.

Taking a look at jsCanvasRenderingContext2DPrototypeFunctionStroke1, we see the argumentCount is once again checked, despite it already having been done in the main function: 

static inline JSC::EncodedJSValue jsCanvasRenderingContext2DPrototypeFunctionStroke1Caller(JSC::ExecState* state, JSCanvasRenderingContext2D* castedThis, JSC::ThrowScope& throwScope)
{
    UNUSED_PARAM(state);
    UNUSED_PARAM(throwScope);
    auto& impl = castedThis->wrapped();
    if (UNLIKELY(state->argumentCount() < 1))
        return throwVMError(state, throwScope, createNotEnoughArgumentsError(state));
    auto path = convert<IDLInterface<DOMPath>>(*state, state->uncheckedArgument(0), [](JSC::ExecState& state, JSC::ThrowScope& scope) { throwArgumentTypeError(state, scope, 0, "path", "CanvasRenderingContext2D", "stroke", "DOMPath"); });
    RETURN_IF_EXCEPTION(throwScope, encodedJSValue());
    impl.stroke(*path);
    return JSValue::encode(jsUndefined());
}


We could further improve things by taking advantage of the cases where disambiguation takes place, and retain the type information into the helpers.
Comment 1 Sam Weinig 2017-05-23 20:07:23 PDT
Created attachment 311093 [details]
Patch
Comment 2 Chris Dumez 2017-05-24 08:42:52 PDT
Comment on attachment 311093 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=311093&action=review

r=me with nit.

> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:4056
> +        foreach my $function (@functions) {

FYI, I'd personally if we used IDL naming consistently in the bindings generator. i.e. function -> operation.

> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:4746
> +    return if $function->{overloads} && @{$function->{overloads}} > 1;

Maybe a comment to explain why we do not need argument count check for overloads?
Comment 3 Sam Weinig 2017-05-24 09:06:51 PDT
(In reply to Chris Dumez from comment #2)
> Comment on attachment 311093 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=311093&action=review
> 
> r=me with nit.
> 
> > Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:4056
> > +        foreach my $function (@functions) {
> 
> FYI, I'd personally if we used IDL naming consistently in the bindings
> generator. i.e. function -> operation.

I'll do this in another pass. Get them all at once.

> 
> > Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:4746
> > +    return if $function->{overloads} && @{$function->{overloads}} > 1;
> 
> Maybe a comment to explain why we do not need argument count check for
> overloads?

Will do.
Comment 4 Sam Weinig 2017-05-24 09:08:33 PDT
Committed r217369: <http://trac.webkit.org/changeset/217369>
Comment 5 Radar WebKit Bug Importer 2017-05-30 20:24:48 PDT
<rdar://problem/32479792>