Bug 183987 - [JSC] Remove repeated iteration of ElementNode
Summary: [JSC] Remove repeated iteration of ElementNode
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Yusuke Suzuki
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2018-03-24 14:46 PDT by Yusuke Suzuki
Modified: 2018-03-27 01:50 PDT (History)
6 users (show)

See Also:


Attachments
Patch (8.55 KB, patch)
2018-03-24 14:52 PDT, Yusuke Suzuki
keith_miller: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Yusuke Suzuki 2018-03-24 14:46:42 PDT
[JSC] Remove repeated iteration of ElementNode
Comment 1 Yusuke Suzuki 2018-03-24 14:52:59 PDT
Created attachment 336486 [details]
Patch
Comment 2 Keith Miller 2018-03-26 16:19:14 PDT
Comment on attachment 336486 [details]
Patch

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

r=me with some nits.

> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:398
> +    auto newArray = [&generator] (RegisterID* dst, ElementNode* elements, unsigned length, bool hadVariableExpression) {

Nit: Why not just capture the args you're using?

> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:418
> +                if (!!node->elision()) {

Nit: You don't need the !!.
Comment 3 Yusuke Suzuki 2018-03-27 01:46:29 PDT
Comment on attachment 336486 [details]
Patch

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

Thank you!

>> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:398
>> +    auto newArray = [&generator] (RegisterID* dst, ElementNode* elements, unsigned length, bool hadVariableExpression) {
> 
> Nit: Why not just capture the args you're using?

I want to separate the responsibility of ArrayNode::emitBytecode and this newArray function.
This newArray does not recognize how `hadVariableExpression` and `length` is computed. Passing it as an argument explicitly makes this separation clear.

>> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:418
>> +                if (!!node->elision()) {
> 
> Nit: You don't need the !!.

Dropped.
Comment 4 Yusuke Suzuki 2018-03-27 01:49:46 PDT
Committed r229993: <https://trac.webkit.org/changeset/229993>
Comment 5 Radar WebKit Bug Importer 2018-03-27 01:50:25 PDT
<rdar://problem/38903107>