556866 - nameinc-type ops don't fill the property cache correctly

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Boris Zbarsky [:bzbarsky]]

Since they have JOF_INCDEC set we end up filling the sprop, not the slot.  But the code in jsinterp for these ops can only use a slot cache entry, so we lose.

It should be safe, we think, to use a slot cache for JOF_INCDEC if the setter is default.

Comment 1

[Brendan Eich [:brendan]]

Yeah, this is one of those cases I left for "later" early in 2008, the Firefox 3 property cache come-back. Thanks for getting to it!

/be

Comment 2

[Boris Zbarsky [:bzbarsky]]

Attachment: Like so, perhaps

Comment 3

[Brendan Eich [:brendan]]

Comment on attachment 436766 [details] [diff] [review]
Like so, perhaps

>+        /* If getting a value via a stub getter, or doing an INCDEC op
>+           with stub getters and setters, we can cache the slot. */

Please use major comment style.

>+        if ((!(cs->format & (JOF_SET | JOF_FOR)) &&
>+             (sprop->hasDefaultSetter() || (!(cs->format & JOF_INCDEC)))) &&

1. The !(cs->format & JOF_INCDEC) expression is overparenthesized against ||.

2. Would it be easier to read to apply De Morgan and write

>+             !((cs->format & JOF_INCDEC) && !sprop->hasDefaultSetter())) &&

or just transpose the terms of your inner ||:

>+             (!(cs->format & JOF_INCDEC) || sprop->hasDefaultSetter())) &&

The fewer double-negatives, the better, I say!

/be

Comment 4

[Boris Zbarsky [:bzbarsky]]

Attachment: Without silly extra parens

Comment 5

[Boris Zbarsky [:bzbarsky]]

Pushed http://hg.mozilla.org/tracemonkey/rev/eb0d26f8598d

Comment 6

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/eb0d26f8598d