Bug 136549 - Merge StructureShapes that share the same prototype chain
Summary: Merge StructureShapes that share the same prototype chain
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Saam Barati
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2014-09-04 14:48 PDT by Saam Barati
Modified: 2014-09-08 11:15 PDT (History)
2 users (show)

See Also:


Attachments
patch (11.61 KB, patch)
2014-09-04 22:28 PDT, Saam Barati
no flags Details | Formatted Diff | Diff
patch (11.78 KB, patch)
2014-09-04 22:40 PDT, Saam Barati
no flags Details | Formatted Diff | Diff
patch (11.60 KB, patch)
2014-09-05 09:37 PDT, Saam Barati
fpizlo: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Saam Barati 2014-09-04 14:48:06 PDT
Create a more sophisticated merge of StructureShapes that will merge objects with the same prototype chain.
Comment 1 Saam Barati 2014-09-04 15:08:55 PDT
(In reply to comment #0)
> Create a more sophisticated merge of StructureShapes that will merge objects with the same prototype chain.

And also create the notion of optional fields.
Comment 2 Saam Barati 2014-09-04 22:28:55 PDT
Created attachment 237674 [details]
patch
Comment 3 WebKit Commit Bot 2014-09-04 22:31:31 PDT
Attachment 237674 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/runtime/TypeSet.cpp:86:  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]
ERROR: Source/JavaScriptCore/runtime/TypeSet.cpp:535:  The parameter type should use PassRefPtr instead of RefPtr.  [readability/pass_ptr] [5]
ERROR: Source/JavaScriptCore/runtime/TypeSet.h:81:  The parameter name "a" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/runtime/TypeSet.h:82:  The parameter type should use PassRefPtr instead of RefPtr.  [readability/pass_ptr] [5]
Total errors found: 4 in 4 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 4 Saam Barati 2014-09-04 22:40:10 PDT
Created attachment 237675 [details]
patch
Comment 5 Filip Pizlo 2014-09-05 07:48:30 PDT
Comment on attachment 237675 [details]
patch

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

> Source/JavaScriptCore/runtime/TypeSet.cpp:69
> +void TypeSet::addTypeInformation(RuntimeType type, PassRefPtr<StructureShape> prpNewShape, StructureID id) 

What's is "prp"?  We usually stay away from abbreviations in variable names.

> Source/JavaScriptCore/runtime/TypeSet.cpp:557
> +    for (auto iter = a->m_fields.begin(), end = a->m_fields.end(); iter != end; ++iter) {

Any reason why these loops aren't C++11 for-in loops, like: for (auto value : a->m_fields) ...?
Comment 6 Saam Barati 2014-09-05 09:08:32 PDT
(In reply to comment #5)
> (From update of attachment 237675 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=237675&action=review
> 
> > Source/JavaScriptCore/runtime/TypeSet.cpp:69
> > +void TypeSet::addTypeInformation(RuntimeType type, PassRefPtr<StructureShape> prpNewShape, StructureID id) 
> 
> What's is "prp"?  We usually stay away from abbreviations in variable names.
I think it stands for pass reference pointer. That's what the webkit article says to prefix variables with that are PassRefPtr but immediately put into RefPtrs with. Should I switch to something else? Usually how I like to handle this is:
function (_foo) { var foo = _foo };

> 
> > Source/JavaScriptCore/runtime/TypeSet.cpp:557
> > +    for (auto iter = a->m_fields.begin(), end = a->m_fields.end(); iter != end; ++iter) {
> 
> Any reason why these loops aren't C++11 for-in loops, like: for (auto value : a->m_fields) ...?

No, I should switch to that.
Comment 7 Saam Barati 2014-09-05 09:37:31 PDT
Created attachment 237695 [details]
patch
Comment 8 Saam Barati 2014-09-08 11:15:19 PDT
landed in: http://trac.webkit.org/changeset/173388