WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
119189
Some cleanup in PropertySlot
https://bugs.webkit.org/show_bug.cgi?id=119189
Summary
Some cleanup in PropertySlot
Gavin Barraclough
Reported
2013-07-27 23:43:27 PDT
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.
Attachments
fix
(12.15 KB, patch)
2013-07-27 23:49 PDT
,
Gavin Barraclough
sam
: review+
Details
Formatted Diff
Diff
fix
(30.73 KB, patch)
2013-07-29 15:44 PDT
,
Gavin Barraclough
ggaren
: review+
Details
Formatted Diff
Diff
Show Obsolete
(1)
View All
Add attachment
proposed patch, testcase, etc.
Gavin Barraclough
Comment 1
2013-07-27 23:49:09 PDT
Created
attachment 207603
[details]
fix
WebKit Commit Bot
Comment 2
2013-07-27 23:52:15 PDT
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 3
2013-07-28 11:09:00 PDT
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 4
2013-07-28 13:23:47 PDT
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
Comment 5
2013-07-29 15:44:31 PDT
Created
attachment 207674
[details]
fix
Gavin Barraclough
Comment 6
2013-07-29 15:45:10 PDT
> Can we ASSERT that the m_propertyType is TypeCustomIndex here?
Cleaned up a bit more - removed this method.
WebKit Commit Bot
Comment 7
2013-07-29 15:45:45 PDT
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
Comment 8
2013-07-29 15:46:32 PDT
> 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 9
2013-07-29 17:08:11 PDT
Comment on
attachment 207674
[details]
fix r=me
Gavin Barraclough
Comment 10
2013-07-29 18:36:29 PDT
Fixed in
r153454
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug