Bug 102326 - Structure should be able to easily tell if the prototype chain might intercept a store
Summary: Structure should be able to easily tell if the prototype chain might intercep...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Filip Pizlo
URL:
Keywords:
Depends on: 102292
Blocks: 102327
  Show dependency treegraph
 
Reported: 2012-11-14 21:25 PST by Filip Pizlo
Modified: 2012-11-15 12:23 PST (History)
8 users (show)

See Also:


Attachments
the patch (6.64 KB, patch)
2012-11-14 21:35 PST, Filip Pizlo
ggaren: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Filip Pizlo 2012-11-14 21:25:03 PST
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.
Comment 1 Filip Pizlo 2012-11-14 21:35:10 PST
Created attachment 174337 [details]
the patch
Comment 2 WebKit Review Bot 2012-11-14 21:38:59 PST
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.
Comment 3 Filip Pizlo 2012-11-14 21:47:23 PST
(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.
Comment 4 Geoffrey Garen 2012-11-15 12:07:36 PST
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.
Comment 5 Filip Pizlo 2012-11-15 12:10:32 PST
(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.
Comment 6 Filip Pizlo 2012-11-15 12:23:01 PST
Landed (with style fix) in http://trac.webkit.org/changeset/134813