Bug 87185 - [V8] Refactor generation code for non-standard functions
Summary: [V8] Refactor generation code for non-standard functions
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Kent Tamura
URL:
Keywords:
Depends on:
Blocks: 87086
  Show dependency treegraph
 
Reported: 2012-05-22 17:04 PDT by Kent Tamura
Modified: 2012-05-22 20:56 PDT (History)
4 users (show)

See Also:


Attachments
Patch (11.16 KB, patch)
2012-05-22 17:11 PDT, Kent Tamura
no flags Details | Formatted Diff | Diff
Patch 2 (11.18 KB, patch)
2012-05-22 17:47 PDT, Kent Tamura
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Kent Tamura 2012-05-22 17:04:13 PDT
[V8] Refactor generation code for non-standard functions
Comment 1 Kent Tamura 2012-05-22 17:11:13 PDT
Created attachment 143403 [details]
Patch
Comment 2 Kentaro Hara 2012-05-22 17:22:59 PDT
Comment on attachment 143403 [details]
Patch

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

IsStandardFunction() looks good, but I am neutral to the refactoring of GenerateNonStandrdFunction(). It seems that GenerateNonStandrdFunction() "just" factors out a code, which would not be so beneficial.

> Source/WebCore/ChangeLog:11
> +        (IsStandardFunction): Introduce a new functio to check if a

Nit: function

> Source/WebCore/ChangeLog:15
> +        (GenerateImplementation): Use IsStandardFunction and GenerateNonStandrdFunction.

Nit: GenerateNonStandardFunction.

> Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:2127
> +sub GenerateNonStandrdFunction

GenerateStandardFunction

> Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:2206
> +        die "This shouldn't happen: " . IsStandardFunction($function) . "\n";

How does this message help? IsStandardFunction($function) would just output 0 or 1?

> Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:-2721
> -            $num_callbacks++;

This line seems to be lost in your patch.

> Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:2762
> +        next if IsStandardFunction($dataNode, $function);

Why is this check needed? It seems to be already checked above.

> Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:2763
> +        GenerateNonStandrdFunction($dataNode, $function);

GenerateNonStandardFunction
Comment 3 Kent Tamura 2012-05-22 17:44:46 PDT
Comment on attachment 143403 [details]
Patch

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

> IsStandardFunction() looks good, but I am neutral to the refactoring of GenerateNonStandrdFunction(). It seems that GenerateNonStandrdFunction() "just" factors out a code, which would not be so beneficial.

I need to add callsite to GenerateNonStandardFunction() in Bug 87086.

>> Source/WebCore/ChangeLog:11
>> +        (IsStandardFunction): Introduce a new functio to check if a
> 
> Nit: function

Done.

>> Source/WebCore/ChangeLog:15
>> +        (GenerateImplementation): Use IsStandardFunction and GenerateNonStandrdFunction.
> 
> Nit: GenerateNonStandardFunction.

Oops, correct all of instances.

>> Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:2206
>> +        die "This shouldn't happen: " . IsStandardFunction($function) . "\n";
> 
> How does this message help? IsStandardFunction($function) would just output 0 or 1?

I'll change it so that it prints $commentInfo.

>> Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:-2721
>> -            $num_callbacks++;
> 
> This line seems to be lost in your patch.

In my new code, $num_callbacks++ is always called except for standard functions.

>> Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:2762
>> +        next if IsStandardFunction($dataNode, $function);
> 
> Why is this check needed? It seems to be already checked above.

Because we need to skip standard functions.  The old code skipped standrad functions after some tasks for non-standard functions.

       if ($template eq "proto" && $conditional eq "" && $signature eq "defaultSignature" && $property_attributes eq "") {
            # Standard type of callback, already created in the batch, so skip it here.
            next;
       }
    # This is equivalent to IsStandardFunction().

Skipping the standard function in such deep place is not good for code readability.
The foreach loop for @{$dataNode->functions} above in this function is for standard functions, and
This foreach loop for @{$dataNode->functions} in this function is for non-standard functions.
The role of each foreach loops becomes clear.
Comment 4 Kent Tamura 2012-05-22 17:47:53 PDT
Created attachment 143415 [details]
Patch 2
Comment 5 Kentaro Hara 2012-05-22 17:53:36 PDT
Comment on attachment 143415 [details]
Patch 2

Looks OK!
Comment 6 Kent Tamura 2012-05-22 17:56:51 PDT
Comment on attachment 143415 [details]
Patch 2

Thank you!
Comment 7 WebKit Review Bot 2012-05-22 20:55:59 PDT
Comment on attachment 143415 [details]
Patch 2

Clearing flags on attachment: 143415

Committed r118113: <http://trac.webkit.org/changeset/118113>
Comment 8 WebKit Review Bot 2012-05-22 20:56:04 PDT
All reviewed patches have been landed.  Closing bug.