Bug 102326 – Structure should be able to easily tell if the prototype chain might intercept a store

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

[Filip Pizlo]

Currently all of that logic is in JSObject::put().  The logic in JSObject::put() is great, because it's well optimized for common cases, but (a) the optimizations are sometimes wrong (see https://bugs.webkit.org/show_bug.cgi?id=102292) and (b) it means that given just the structure, you can't predict if interception will happen.

[Filip Pizlo]

Created attachment 174337 [details]
the patch

[WebKit Review Bot]

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/ChangeLog', u'Source..." exit_code: 1
Source/JavaScriptCore/runtime/JSObject.cpp:368:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
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]

(In reply to comment #2)
> Attachment 174337 [details] did not pass style-queue:
> 
> Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/ChangeLog', u'Source..." exit_code: 1
> Source/JavaScriptCore/runtime/JSObject.cpp:368:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
> Total errors found: 1 in 4 files
> 
> 
> If any of these errors are false positives, please file a bug against check-webkit-style.

I kind of feel like the style I used here is appropriate.  I can change it (by not wrapping the line), but I quite like it.

Basically I have:

if (somethingLong(moreLongThings) && anotherThing)
    foo;

And I wrapped it to:

if (somethingLong(
        moreLongThings)
    && anotherThing)
    foo;

While the style checker wants:

if (somethingLong(
    moreLongThings)
    && anotherThing)
    foo;

To me, the latter is super awkward while the former (what I did) is quite natural.

If the reviewer wants it changed, I'll change it.  Otherwise I'll consider this a random case of the style checker not being awesome.

[Geoffrey Garen]

Comment on attachment 174337 [details]
the patch

r=me

Please do the wrapped line in the current style way.

I think most people see this particular case as a bad case in our current guideline, so maybe you can suggest this new way in email, and persuade people.

[Filip Pizlo]

(In reply to comment #4)
> (From update of attachment 174337 [details])
> r=me
> 
> Please do the wrapped line in the current style way.
> 
> I think most people see this particular case as a bad case in our current guideline, so maybe you can suggest this new way in email, and persuade people.

I'll just unwrap the line.  Your argument that the current style reflects what most auto-indenters do is pretty convincing to me.

[Filip Pizlo]

Landed (with style fix) in http://trac.webkit.org/changeset/134813