Summary: | Array index miss case creates a string every time | ||||||
---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Darin Adler <darin> | ||||
Component: | JavaScriptCore | Assignee: | Darin Adler <darin> | ||||
Status: | RESOLVED FIXED | ||||||
Severity: | Enhancement | CC: | ddkilzer | ||||
Priority: | P2 | ||||||
Version: | 528+ (Nightly build) | ||||||
Hardware: | All | ||||||
OS: | All | ||||||
Attachments: |
|
Description
Darin Adler
2009-08-22 23:33:26 PDT
Created attachment 38446 [details]
patch
Comment on attachment 38446 [details]
patch
Maybe it should be called m_mayHaveIntegralNamedProperty because I don't have any code to set it to false if the offending properties are all removed.
Comment on attachment 38446 [details]
patch
Or I could reverse its sense and call this m_hasNoIntegralNamedProperties.
Comment on attachment 38446 [details] patch >+ if (isUInt32) >+ m_hasIntegralNamedProperty = true; You could save a branch here by always assigning m_hasIntegralNamedProperty: m_hasIntegralNamedProperty = isUInt32; Not sure if that's helpful or not, though. (In reply to comment #4) > (From update of attachment 38446 [details]) > >+ if (isUInt32) > >+ m_hasIntegralNamedProperty = true; > > You could save a branch here by always assigning m_hasIntegralNamedProperty: > > m_hasIntegralNamedProperty = isUInt32; > > Not sure if that's helpful or not, though. That would break the case where isUInt32 is false and m_hasIntegralNamedProperty is already true, but I could do: m_hasIntegralNamedProperty |= isUInt32; Or I could skip the entire check if m_hasIntegralNamedProperty is already true. No need to call toStrictUInt32 at all in that case. But none of that seems important. We don't need to optimize adding properties to the object prototypes. We should optimize for clarity. My existing patch seems OK to me, but we could make changes. Comment on attachment 38446 [details] patch > + return m_hasIntegralNamedProperty && JSObject::getOwnPropertySlot(exec, propertyName, slot); I tend to think that code like this is more readable if the m_hasIntegralNamedProperty case is an explicit early return. Up to you. r=me |