Overloads auto-generation in V8
Created attachment 53016 [details] Patch
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.
Attachment 53016 [details] did not build on qt: Build output: http://webkit-commit-queue.appspot.com/results/1626339
Attachment 53016 [details] did not build on chromium: Build output: http://webkit-commit-queue.appspot.com/results/1655273
Attachment 53016 [details] did not build on gtk: Build output: http://webkit-commit-queue.appspot.com/results/1577375
Created attachment 53175 [details] Patch
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.
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 :(
Created attachment 53393 [details] Patch
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.
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.
Comment on attachment 53393 [details] Patch Awesome, thanks!
Comment on attachment 53393 [details] Patch Clearing flags on attachment: 53393 Committed r57653: <http://trac.webkit.org/changeset/57653>
All reviewed patches have been landed. Closing bug.