Bug 136983 – Structure should have a method for concurrently getting all of the property map entries, and this method shouldn't involve copy-paste

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Filip Pizlo]

Patch forthcoming.

[Filip Pizlo]

Created attachment 238429 [details]
the patch

[WebKit Commit Bot]

Attachment 238429 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/runtime/Structure.cpp:1115:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
Total errors found: 1 in 4 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Filip Pizlo]

Created attachment 238431 [details]
the patch

For real this time.

[Mark Hahnenberg]

Comment on attachment 238431 [details]
the patch

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

r=me with comments.

> Source/JavaScriptCore/runtime/Structure.h:76
> +struct PropertyMapEntry {

Maybe it makes sense to just pull this into its own header? It seems sort of weird to move it to Structure.h from PropertyMapHashTable.h.

> Source/JavaScriptCore/runtime/StructureInlines.h:117
> +        for (auto entry : *table) {

auto& maybe? Do we want to copy each entry as we're iterating?

[Filip Pizlo]

(In reply to comment #4)
> (From update of attachment 238431 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=238431&action=review
> 
> r=me with comments.
> 
> > Source/JavaScriptCore/runtime/Structure.h:76
> > +struct PropertyMapEntry {
> 
> Maybe it makes sense to just pull this into its own header? It seems sort of weird to move it to Structure.h from PropertyMapHashTable.h.

I would love to be able to do that.  Structure.h is included by everything in WebKit, so moving PropertyMapEntry into another head and including it from Structure.h would mean that there would be one more header that is included by everything in WebKit.  Last I checked, such things make kling and smfr sad.

> 
> > Source/JavaScriptCore/runtime/StructureInlines.h:117
> > +        for (auto entry : *table) {
> 
> auto& maybe? Do we want to copy each entry as we're iterating?

Lol OK.  In this case it can't possibly matter.  The body of the loop will use the elements on entry.  If we use auto without the & it means that initially clang will emit loads for all of the elements of PropertyMapEntry, which are all PODs so these are really just simple loads, and then llvm will DCE the loads that you didn't use.  If you use auto&, then clang will emit the loads at the point of use.  It doesn't matter.

[Filip Pizlo]

Landed in http://trac.webkit.org/changeset/173799