Bug 119189 – Some cleanup in PropertySlot

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

[Gavin Barraclough]

PropertySlot represents a property in one of four states - value, getter, custom, or custom-index.  The state is currently tracked redundantly by two mechanisms - the custom getter function (m_getValue) is set to a special value to indicate the type (other than custom), and the type is also tracked by an enum - but only if cacheable.  Cacheability can typically be determined by the value of m_offset (this is invalidOffset if not cacheable).

* Internally, always track the type of the property using an enum value, PropertyType.
* Use m_offset to indicate cacheable.
* Keep the external interface (CachedPropertyType) unchanged.
* Better pack data into the m_data union.

[Gavin Barraclough]

Created attachment 207603 [details]
fix

[WebKit Commit Bot]

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/ChangeLog', u'Source/JavaScriptCore/runtime/PropertySlot.cpp', u'Source/JavaScriptCore/runtime/PropertySlot.h']" exit_code: 1
Source/JavaScriptCore/ChangeLog:14:  Need whitespace between colon and description  [changelog/filechangedescriptionwhitespace] [5]
Source/JavaScriptCore/ChangeLog:15:  Need whitespace between colon and description  [changelog/filechangedescriptionwhitespace] [5]
Source/JavaScriptCore/ChangeLog:16:  Need whitespace between colon and description  [changelog/filechangedescriptionwhitespace] [5]
Source/JavaScriptCore/ChangeLog:17:  Need whitespace between colon and description  [changelog/filechangedescriptionwhitespace] [5]
Total errors found: 4 in 3 files


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

[Sam Weinig]

Comment on attachment 207603 [details]
fix

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

> Source/JavaScriptCore/runtime/PropertySlot.h:205
> +        unsigned index() const { return m_data.customIndex.index; }

Can we ASSERT that the m_propertyType is TypeCustomIndex here?

[Geoffrey Garen]

Comment on attachment 207603 [details]
fix

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

> Source/JavaScriptCore/runtime/PropertySlot.h:50
>          enum CachedPropertyType {
> -            Uncacheable,
> -            Getter,
> -            Custom,
> -            Value
> +            Uncacheable = TypeUnset,
> +            Value = TypeValue,
> +            Getter = TypeGetter,
> +            Custom = TypeCustom

Can you do something to help distinguish these names as being the cacheable variants? Maybe "CacheableValue", etc.?

[Gavin Barraclough]

Created attachment 207674 [details]
fix

[Gavin Barraclough]

> Can we ASSERT that the m_propertyType is TypeCustomIndex here?

Cleaned up a bit more - removed this method.

[WebKit Commit Bot]

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/ChangeLog', u'Source/JavaScriptCore/dfg/DFGRepatch.cpp', u'Source/JavaScriptCore/jit/JITPropertyAccess.cpp', u'Source/JavaScriptCore/jit/JITPropertyAccess32_64.cpp', u'Source/JavaScriptCore/jit/JITStubs.cpp', u'Source/JavaScriptCore/llint/LLIntSlowPaths.cpp', u'Source/JavaScriptCore/runtime/PropertySlot.cpp', u'Source/JavaScriptCore/runtime/PropertySlot.h']" exit_code: 1
Source/JavaScriptCore/ChangeLog:14:  Need whitespace between colon and description  [changelog/filechangedescriptionwhitespace] [5]
Source/JavaScriptCore/ChangeLog:15:  Need whitespace between colon and description  [changelog/filechangedescriptionwhitespace] [5]
Source/JavaScriptCore/ChangeLog:16:  Need whitespace between colon and description  [changelog/filechangedescriptionwhitespace] [5]
Source/JavaScriptCore/ChangeLog:17:  Need whitespace between colon and description  [changelog/filechangedescriptionwhitespace] [5]
Total errors found: 4 in 8 files


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

[Gavin Barraclough]

> Can you do something to help distinguish these names as being the cacheable variants? Maybe "CacheableValue", etc.?

Was going to tidy up the external interface in a follow up patch, but was simple enough.  Removed this enum, added more methods matching existing isCacheableValue() instead.

[Geoffrey Garen]

Comment on attachment 207674 [details]
fix

r=me

[Gavin Barraclough]

Fixed in r153454