Bug 37373 – Overloads auto-generation in V8

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Yaar Schnitman]

Overloads auto-generation in V8

[Yaar Schnitman]

Created attachment 53016 [details]
Patch

[WebKit Review Bot]

Attachment 53016 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebCore/bindings/v8/test/V8TestObj.cpp:268:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
WebCore/bindings/v8/test/V8TestObj.cpp:420:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 2 in 5 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Early Warning System Bot]

Attachment 53016 [details] did not build on qt:
Build output: http://webkit-commit-queue.appspot.com/results/1626339

[WebKit Review Bot]

Attachment 53016 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/1655273

[WebKit Review Bot]

Attachment 53016 [details] did not build on gtk:
Build output: http://webkit-commit-queue.appspot.com/results/1577375

[Yaar Schnitman]

Created attachment 53175 [details]
Patch

[WebKit Review Bot]

Attachment 53175 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebCore/bindings/v8/test/V8TestObj.cpp:268:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
WebCore/bindings/v8/test/V8TestObj.cpp:277:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
WebCore/bindings/v8/test/V8TestObj.cpp:290:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
WebCore/bindings/v8/test/V8TestObj.cpp:298:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
WebCore/bindings/v8/test/V8TestObj.cpp:306:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
WebCore/bindings/v8/test/V8TestObj.cpp:308:  An else if statement should be written as an if statement when the prior "if" concludes with a return, break, continue or goto statement.  [readability/control_flow] [4]
WebCore/bindings/v8/test/V8TestObj.cpp:465:  overloadedMethod_argc is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
WebCore/bindings/v8/test/V8TestObj.cpp:466:  overloadedMethod_argv is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
WebCore/bindings/v8/test/V8TestObj.cpp:467:  overloadedMethod_signature is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Total errors found: 9 in 5 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Nate Chapin]

Comment on attachment 53175 [details]
Patch

The core of this patch looks right, just a couple of small changes + comment requests in the code generator changes.

Also, please forgive my grammar nitpicks...

> +        This will be used by XHR.send/open, Canvas.*, WebGL.* methods that are currently custom. When more then a single overload exists for a method, the right overload is determined based on total number of arguments passed as well as the values passed to non-primitive arguments.

"When more then" -> "When more than" ?

> Overloads better be specified from the most precise (overloads with wrapper type arguments) to the least precise (overloads with only primitive type arguments).

This wording confuses me slightly....did you mean "Overloads need to be..."?


> -    return 1 if $primitiveTypeHash{$type};
> +    return 1 if $nonNumberPrimitiveTypeHash{$type} || $numberPrimitiveTypeHash{$type};
> +    return 0;
> +}
> +
> +sub IsNumberType
> +{
> +    my $object = shift;
> +    my $type = shift;
> +
> +    return 1 if $numberPrimitiveTypeHash{$type};
>      return 0;
>  }

I don't think IsNumberType is called anywhere, and it's a duplicate of $numberPrimitiveTypeHash{$type} anyway.  Perhaps a IsPrimitiveType that checks both $numberPrimitiveTypeHash and $nonNumberPrimitiveTypeHash instead?


>      my $dataNode = shift;
>      my $defines = shift;
>  
> +    LinkOverloadedFunctions($dataNode);
> +
>      # Start actual generation
>      $object->GenerateHeader($dataNode);
>      $object->GenerateImplementation($dataNode);

Can this call to LinkOverloadedFunctions go in GenerateImplementation? If I'm reading it correctly, it won't ever affect the .h file output.



> +    if (@{$function->{overloads}} > 1) {
> +        $name = $name . $function->{overloadIndex};
> +    }
> +

Would you mind commenting this?


> @@ -1624,6 +1723,7 @@ sub GenerateImplementation
>      $num_callbacks = 0;
>      $has_callbacks = 0;
>      foreach my $function (@{$dataNode->functions}) {
> +        next if $function->{overloadIndex} > 1;
>          my $attrExt = $function->signature->extendedAttributes;
>          # Don't put any nonstandard functions into this table:
>          if ($attrExt->{"V8OnInstance"}) {
> @@ -1785,6 +1885,7 @@ END
>      # Define our functions with Set() or SetAccessor()
>      $total_functions = 0;
>      foreach my $function (@{$dataNode->functions}) {
> +        next if $function->{overloadIndex} > 1;
>          $total_functions++;
>          my $attrExt = $function->signature->extendedAttributes;
>          my $name = $function->signature->name;

Same for these, please comment why this is necessary. I'm going to forget otherwise :(

[Yaar Schnitman]

Created attachment 53393 [details]
Patch

[Yaar Schnitman]

Comment on attachment 53393 [details]
Patch

I undid the changes in CodeGenerator.pm, since I ended up not using them (as you noticed yourself). Rest of feedback is incorporated. Please cq+ if LGTM.

[WebKit Review Bot]

Attachment 53393 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebCore/bindings/v8/test/V8TestObj.cpp:268:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
WebCore/bindings/v8/test/V8TestObj.cpp:277:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
WebCore/bindings/v8/test/V8TestObj.cpp:290:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
WebCore/bindings/v8/test/V8TestObj.cpp:298:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
WebCore/bindings/v8/test/V8TestObj.cpp:306:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
WebCore/bindings/v8/test/V8TestObj.cpp:308:  An else if statement should be written as an if statement when the prior "if" concludes with a return, break, continue or goto statement.  [readability/control_flow] [4]
WebCore/bindings/v8/test/V8TestObj.cpp:465:  overloadedMethod_argc is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
WebCore/bindings/v8/test/V8TestObj.cpp:466:  overloadedMethod_argv is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
WebCore/bindings/v8/test/V8TestObj.cpp:467:  overloadedMethod_signature is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Total errors found: 9 in 4 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Nate Chapin]

Comment on attachment 53393 [details]
Patch

Awesome, thanks!

[WebKit Commit Bot]

Comment on attachment 53393 [details]
Patch

Clearing flags on attachment: 53393

Committed r57653: <http://trac.webkit.org/changeset/57653>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.