Bug 136549 – Merge StructureShapes that share the same prototype chain

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Saam Barati]

Create a more sophisticated merge of StructureShapes that will merge objects with the same prototype chain.

[Saam Barati]

(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.

[Saam Barati]

Created attachment 237674 [details]
patch

[WebKit Commit Bot]

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.

[Saam Barati]

Created attachment 237675 [details]
patch

[Filip Pizlo]

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) ...?

[Saam Barati]

(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.

[Saam Barati]

Created attachment 237695 [details]
patch

[Saam Barati]

landed in: http://trac.webkit.org/changeset/173388