Bug 158835 - Templatize JS bindings code generator of functions with variadic parameters
Summary: Templatize JS bindings code generator of functions with variadic parameters
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Bindings (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nael Ouedraogo
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2016-06-16 06:10 PDT by Nael Ouedraogo
Modified: 2016-09-07 03:05 PDT (History)
7 users (show)

See Also:


Attachments
Patch (13.16 KB, patch)
2016-06-16 06:49 PDT, Nael Ouedraogo
no flags Details | Formatted Diff | Diff
Patch (17.75 KB, patch)
2016-06-23 03:47 PDT, Nael Ouedraogo
no flags Details | Formatted Diff | Diff
Patch (17.51 KB, patch)
2016-06-27 07:03 PDT, Nael Ouedraogo
no flags Details | Formatted Diff | Diff
Patch (17.75 KB, patch)
2016-07-11 11:05 PDT, Nael Ouedraogo
no flags Details | Formatted Diff | Diff
Patch (16.85 KB, patch)
2016-08-30 06:55 PDT, Nael Ouedraogo
no flags Details | Formatted Diff | Diff
Patch (18.00 KB, patch)
2016-09-07 01:49 PDT, Nael Ouedraogo
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Nael Ouedraogo 2016-06-16 06:10:02 PDT
Templatize JS bindings code generator of functions with variadic parameters.
Comment 1 Nael Ouedraogo 2016-06-16 06:49:09 PDT
Created attachment 281454 [details]
Patch
Comment 2 Nael Ouedraogo 2016-06-16 08:38:40 PDT
One possible improvement is to homogenize toNativeArgument() and toDOMArgumentsVariadic() functions. A second improvement is to process both native and DOM arguments with a single function in JS bindings code generator.  

This could be addressed in a follow-up bug.
Comment 3 Nael Ouedraogo 2016-06-23 03:47:59 PDT
Created attachment 281905 [details]
Patch
Comment 4 Nael Ouedraogo 2016-06-23 05:49:21 PDT
In the uploaded patch, variadic arguments (native or DOM) are handled by a single template function.
Comment 5 youenn fablet 2016-06-23 06:29:48 PDT
Comment on attachment 281905 [details]
Patch

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

> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:3686
> +            }

Could it be done as a one liner?
Comment 6 youenn fablet 2016-06-23 06:29:50 PDT
Comment on attachment 281905 [details]
Patch

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

> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:3686
> +            }

Could it be done as a one liner?
Comment 7 Nael Ouedraogo 2016-06-23 06:42:57 PDT
(In reply to comment #6)
> https://bugs.webkit.org/attachment.cgi?id=281905&action=review
> 
> > Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:3686
> > +            }
> 
> Could it be done as a one liner?
Thanks for the review.

I will replace if/else block with a call to ternary operator.
Comment 8 Nael Ouedraogo 2016-06-27 07:03:15 PDT
Created attachment 282128 [details]
Patch
Comment 9 Nael Ouedraogo 2016-06-28 00:43:20 PDT
(In reply to comment #6)
> Comment on attachment 281905 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=281905&action=review
> 
> > Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:3686
> > +            }
> 
> Could it be done as a one liner?

Proposed modification is done in the uploaded patch.
Comment 10 youenn fablet 2016-06-28 00:58:53 PDT
Comment on attachment 282128 [details]
Patch

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

Sounds good to me. Some proposed improvements below.

> Source/WebCore/bindings/js/JSDOMBinding.h:319
> +        if (!TraitsType::nativeValue(&state, jsValue, indexValue))

Would you be able to modify nativeValue to take an ExecState reference as a preliminary patch?

> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:3685
> +            my $domType = IsNativeType($type) ? GetNativeType($interface, $type) : "$type";

Why "$type" and not $type?

> Source/WebCore/bindings/scripts/test/JS/JSTestObj.cpp:5953
> +    auto tail = toArguments<VariadicHelper<JSdouble, double>>(*state, 1);

This code seems odd, since JSdouble probably does not exist.
Maybe replace it by JSC::JSValue for all native types, since it is not used anyway for them?
Comment 11 Nael Ouedraogo 2016-06-30 07:03:59 PDT
I just ad(In reply to comment #10)
> > Source/WebCore/bindings/js/JSDOMBinding.h:319
> > +        if (!TraitsType::nativeValue(&state, jsValue, indexValue))
> 
> Would you be able to modify nativeValue to take an ExecState reference as a
> preliminary patch?

Ok. A preliminary patch is proposed in Bug 159298.

> > Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:3685
> > +            my $domType = IsNativeType($type) ? GetNativeType($interface, $type) : "$type";
> 
> Why "$type" and not $type?
> 
> > Source/WebCore/bindings/scripts/test/JS/JSTestObj.cpp:5953
> > +    auto tail = toArguments<VariadicHelper<JSdouble, double>>(*state, 1);
> 
> This code seems odd, since JSdouble probably does not exist.
> Maybe replace it by JSC::JSValue for all native types, since it is not used
> anyway for them?

I will fix this in a new patch once preliminary Bug 159298 is resolved.
Comment 12 Nael Ouedraogo 2016-07-11 11:05:18 PDT
Created attachment 283325 [details]
Patch
Comment 13 Nael Ouedraogo 2016-07-11 11:11:15 PDT
Preliminary patch is landed (modification of nativeValue to takes an ExecState reference). Uploaded patch fixed also the two remaining issues.
Comment 14 Darin Adler 2016-07-13 09:42:43 PDT
Comment on attachment 283325 [details]
Patch

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

> Source/WebCore/bindings/js/JSDOMBinding.h:313
> +    using Result = typename std::pair<Optional<Container>, size_t>;

Annoying that this is repeated in both versions of VariadicHelper. Might be nice to use inheritance instead.

Since a pair or tuple here is a necessary evil, I suppose we should have the index first and then the value, rather than the value first and then the index. Think of it as an item in a numbered list.

> Source/WebCore/bindings/js/JSDOMBinding.h:315
> +    static Optional<DOMClass> argumentConverter(JSC::ExecState& state, JSC::JSValue jsValue)

Normally we use a verb phrase for a function or a noun phrase that describes the result. It’s not so common to use a noun phrase that describes the function itself. A better name for this would be convert or unwrap.

> Source/WebCore/bindings/js/JSDOMBinding.h:330
> +    using Class     = typename std::remove_reference<decltype(std::declval<JSClass>().wrapped())>::type;
> +    using Item      = std::reference_wrapper<Class>;
> +    using Container = Vector<Item>;
> +    using Result    = typename std::pair<Optional<Container>, size_t>;

We don’t do this kind of vertical alignment formatting in WebKit; style guide specifically forbids it.

> Source/WebCore/bindings/js/JSDOMBinding.h:791
> +        return {Nullopt, 0};

In WebKit coding style there is a space inside the { }.

> Source/WebCore/bindings/js/JSDOMBinding.h:799
> +            return {Nullopt, i};

Ditto.

> Source/WebCore/bindings/js/JSDOMBinding.h:800
> +        result.uncheckedAppend(*value);

We normally use the value() function instead of * with Optional in WebKit code, not entirely sure why.

This should be WTFMove(*value.value()) because in some cases, such as String, that is more efficient than copying.

> Source/WebCore/bindings/js/JSDOMBinding.h:802
> +    return {WTFMove(result), length};

Space here.

> Source/WebCore/bindings/js/JSDOMWrapper.h:88
> +    static constexpr bool isWrappingDOMObject = true;

Terminology here is not consistent. Above we say isSimpleWrapper and isComplexWrapper. This one should be isWrapper. But also this is redundant with isBuiltin; do we really need both?

> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:3709
> +            my $domType = IsNativeType($type) ? GetNativeType($interface, $type) : $type;
> +            my $jsType = IsNativeType($type) ? "JSC::JSValue" : "JS$domType";

I don’t think the names "domType" and "jsType" are good here even though that’s what’s used int he C++ code. I would probably call these "wrapped type" and "wrapper type". Also, since this is the only place in the entire file that uses IsNativeType, it seems like we could change it around to be better for this purpose. It would be more consistent with the other code in this file to have a function that implements this algorithm instead of writing it out like this. It seems likely that the algorithm here is only simple right now because of the many types that we don’t handle. For example, if we wanted this to work properly for enumerations or dictionaries this code would likely have to change. So I suggest coming up with some named functions for this instead of writing the code in here. Maybe a single function with two results.

> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:3718
> +            push(@$outputArray, "    if (!$name.first)\n");
> +            push(@$outputArray, "        return throwArgumentTypeError(*state, $name.second, \"$name\", \"$interfaceName\", $quotedFunctionName, \"$type\");\n");

Seems a shame that we now generate unreachable code to throw an error that can never happen in a case like this. Or is this reachable? If it is reachable, can we add a test case to show the change in behavior?
Comment 15 Nael Ouedraogo 2016-08-30 06:55:11 PDT
Created attachment 287389 [details]
Patch
Comment 16 Nael Ouedraogo 2016-08-30 07:07:46 PDT
Thanks for the review and sorry for the late response.
Coding style mistakes and terminology have been corrected in the uploaded patch. 

> > Source/WebCore/bindings/js/JSDOMBinding.h:313
> > +    using Result = typename std::pair<Optional<Container>, size_t>;
> 
> Annoying that this is repeated in both versions of VariadicHelper. Might be
> nice to use inheritance instead.
OK, repeated code has been factorized.

> > Source/WebCore/bindings/js/JSDOMBinding.h:800
> > +        result.uncheckedAppend(*value);
> 
> We normally use the value() function instead of * with Optional in WebKit
> code, not entirely sure why.
> 
> This should be WTFMove(*value.value()) because in some cases, such as
> String, that is more efficient than copying.
Actually, value was not necessarily an Optional. Return types of
convert methods have been changed to Optional in uploaded patch so that value() can be used. 

> > Source/WebCore/bindings/js/JSDOMWrapper.h:88
> > +    static constexpr bool isWrappingDOMObject = true;
> 
> Terminology here is not consistent. Above we say isSimpleWrapper and
> isComplexWrapper. This one should be isWrapper. But also this is redundant
> with isBuiltin; do we really need both?
Yes, it can be removed since not essential.

> > Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:3709
> > +            my $domType = IsNativeType($type) ? GetNativeType($interface, $type) : $type;
> > +            my $jsType = IsNativeType($type) ? "JSC::JSValue" : "JS$domType";
> 
> I don’t think the names "domType" and "jsType" are good here even though
> that’s what’s used int he C++ code. I would probably call these "wrapped
> type" and "wrapper type". Also, since this is the only place in the entire
> file that uses IsNativeType, it seems like we could change it around to be
> better for this purpose. It would be more consistent with the other code in
> this file to have a function that implements this algorithm instead of
> writing it out like this. It seems likely that the algorithm here is only
> simple right now because of the many types that we don’t handle. For
> example, if we wanted this to work properly for enumerations or dictionaries
> this code would likely have to change. So I suggest coming up with some
> named functions for this instead of writing the code in here. Maybe a single
> function with two results.
>
A function has been added in the uploaded patch with wrapper/wrapped terminology.

> > Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:3718
> > +            push(@$outputArray, "    if (!$name.first)\n");
> > +            push(@$outputArray, "        return throwArgumentTypeError(*state, $name.second, \"$name\", \"$interfaceName\", $quotedFunctionName, \"$type\");\n");
> 
> Seems a shame that we now generate unreachable code to throw an error that
> can never happen in a case like this. Or is this reachable? If it is
> reachable, can we add a test case to show the change in behavior?
I modified the code to avoid generating this unreachable code for native types.
Comment 17 Darin Adler 2016-09-03 12:46:37 PDT
Comment on attachment 287389 [details]
Patch

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

> Source/WebCore/bindings/js/JSDOMBinding.h:321
> +template <typename JSClass, typename DOMClass, typename Enable = void>

We don’t leave a space between "template" and "<", although we don’t use the style consistently. Probably need to mention it in the style guideline.

> Source/WebCore/bindings/js/JSDOMBinding.h:335
> +template <typename JSClass, typename DOMClass>

We don’t leave a space between "template" and "<", although we don’t use the style consistently. Probably need to mention it in the style guideline.

> Source/WebCore/bindings/js/JSDOMBinding.h:353
> +    using Result = typename std::pair<size_t, Optional<Container>>;

Hard to read code that uses pair like this, since the names "first" and "second" aren’t so great. I suppose it wouldn’t be any better with std::tuple, but it could be better if we just used a struct instead because we could then name the members. We made this change a while for HashMap::AddResult and for HashMap::KeyValuePairType; it was a lot more readable to use a struct, and in modern C++ where we are doing anything generic with the pair, the code generated is nearly identical.

> Source/WebCore/bindings/js/JSDOMBinding.h:356
> +template <typename VariadicHelper> typename VariadicHelper::Result toArguments(JSC::ExecState&, size_t startIndex = 0);

We don’t leave a space between "template" and "<", although we don’t use the style consistently. Probably need to mention it in the style guideline.

> Source/WebCore/bindings/js/JSDOMBinding.h:801
> +template <typename VariadicHelper> typename VariadicHelper::Result toArguments(JSC::ExecState& state, size_t startIndex)

We don’t leave a space between "template" and "<", although we don’t use the style consistently. Probably need to mention it in the style guideline.

> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:4582
> +sub GetVariadicType
> +{
> +    my ($interface, $type) = @_;
> +
> +    my $wrappedType = IsNativeType($type) ? GetNativeType($interface, $type) : $type;
> +    my $wrapperType = IsNativeType($type) ? "JSC::JSValue" : "JS$wrappedType";
> +
> +    return ($wrapperType, $wrappedType);
> +}

Really unfortunate that we have to add this helper. We are trying to do these things in the C++ code whenever possible rather than in the code generator. But I don’t have a good idea for how to avoid this.
Comment 18 Nael Ouedraogo 2016-09-07 01:49:15 PDT
Created attachment 288122 [details]
Patch
Comment 19 Nael Ouedraogo 2016-09-07 02:42:40 PDT
Code style errors have been fixed in uploaded patch.

> > Source/WebCore/bindings/js/JSDOMBinding.h:353
> > +    using Result = typename std::pair<size_t, Optional<Container>>;
> 
> Hard to read code that uses pair like this, since the names "first" and
> "second" aren’t so great. I suppose it wouldn’t be any better with
> std::tuple, but it could be better if we just used a struct instead because
> we could then name the members. We made this change a while for
> HashMap::AddResult and for HashMap::KeyValuePairType; it was a lot more
> readable to use a struct, and in modern C++ where we are doing anything
> generic with the pair, the code generated is nearly identical.
> 
Ok, I will fix this in a follow up bug.


Thanks for the review.
Comment 20 WebKit Commit Bot 2016-09-07 03:05:42 PDT
Comment on attachment 288122 [details]
Patch

Clearing flags on attachment: 288122

Committed r205542: <http://trac.webkit.org/changeset/205542>
Comment 21 WebKit Commit Bot 2016-09-07 03:05:49 PDT
All reviewed patches have been landed.  Closing bug.