Changeset 236018 in webkit

Timestamp:

Sep 14, 2018 2:00:21 PM (6 years ago)

Author:

mark.lam@apple.com

Message:

Refactor some ForInContext code for better encapsulation.
​https://bugs.webkit.org/show_bug.cgi?id=189626
<rdar://problem/44466415>

Reviewed by Keith Miller.

1. Add a ForInContext::m_type field to store the context type. This does not increase the class size, but eliminates the need for a virtual call to get the type.

Note: we still need a virtual destructor because we'll be mingling
IndexedForInContexts and StructureForInContexts in the BytecodeGenerator::m_forInContextStack.

2. Add ForInContext::isIndexedForInContext() and ForInContext::isStructureForInContext() convenience methods.

3. Add ForInContext::asIndexedForInContext() and ForInContext::asStructureForInContext() to do the casting to the subclass types. This ensures that we'll properly assert that the casting is legal.

• bytecompiler/BytecodeGenerator.cpp:

(JSC::BytecodeGenerator::emitGetByVal):
(JSC::BytecodeGenerator::popIndexedForInScope):
(JSC::BytecodeGenerator::popStructureForInScope):

• bytecompiler/BytecodeGenerator.h:

(JSC::ForInContext::type const):
(JSC::ForInContext::isIndexedForInContext const):
(JSC::ForInContext::isStructureForInContext const):
(JSC::ForInContext::asIndexedForInContext):
(JSC::ForInContext::asStructureForInContext):
(JSC::ForInContext::ForInContext):
(JSC::StructureForInContext::StructureForInContext):
(JSC::IndexedForInContext::IndexedForInContext):
(JSC::ForInContext::~ForInContext): Deleted.

Location:

trunk/Source/JavaScriptCore

Files:

• ChangeLog (modified) (1 diff)
• bytecompiler/BytecodeGenerator.cpp (modified) (3 diffs)
• bytecompiler/BytecodeGenerator.h (modified) (4 diffs)

trunk/Source/JavaScriptCore/ChangeLog

@@ +1 @@
+2018-09-14  Mark Lam  <mark.lam@apple.com>
+
+        Refactor some ForInContext code for better encapsulation.
+        https://bugs.webkit.org/show_bug.cgi?id=189626
+        <rdar://problem/44466415>
+
+        Reviewed by Keith Miller.
+
+        1. Add a ForInContext::m_type field to store the context type.  This does not
+           increase the class size, but eliminates the need for a virtual call to get the
+           type.
+
+           Note: we still need a virtual destructor because we'll be mingling
+           IndexedForInContexts and StructureForInContexts in the BytecodeGenerator::m_forInContextStack.
+
+        2. Add ForInContext::isIndexedForInContext() and ForInContext::isStructureForInContext()
+           convenience methods.
+
+        3. Add ForInContext::asIndexedForInContext() and ForInContext::asStructureForInContext()
+           to do the casting to the subclass types.  This ensures that we'll properly
+           assert that the casting is legal.
+
+        * bytecompiler/BytecodeGenerator.cpp:
+        (JSC::BytecodeGenerator::emitGetByVal):
+        (JSC::BytecodeGenerator::popIndexedForInScope):
+        (JSC::BytecodeGenerator::popStructureForInScope):
+        * bytecompiler/BytecodeGenerator.h:
+        (JSC::ForInContext::type const):
+        (JSC::ForInContext::isIndexedForInContext const):
+        (JSC::ForInContext::isStructureForInContext const):
+        (JSC::ForInContext::asIndexedForInContext):
+        (JSC::ForInContext::asStructureForInContext):
+        (JSC::ForInContext::ForInContext):
+        (JSC::StructureForInContext::StructureForInContext):
+        (JSC::IndexedForInContext::IndexedForInContext):
+        (JSC::ForInContext::~ForInContext): Deleted.
+
 2018-09-14  Devin Rousso  <webkit@devinrousso.com>
 

trunk/Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp

@@ -2913 +2913 @@
         unsigned instIndex = instructions().size();
 
-        if (context.type() == ForInContext::IndexedForInContextType) {
-            static_cast<IndexedForInContext&>(context).addGetInst(instIndex, property->index());
-            property = static_cast<IndexedForInContext&>(context).index();
+        if (context.isIndexedForInContext()) {
+            auto& indexedContext = context.asIndexedForInContext();
+            indexedContext.addGetInst(instIndex, property->index());
+            property = indexedContext.index();
             break;
         }
 
-        ASSERT(context.type() == ForInContext::StructureForInContextType);
-        StructureForInContext& structureContext = static_cast<StructureForInContext&>(context);
+        StructureForInContext& structureContext = context.asStructureForInContext();
         UnlinkedValueProfile profile = emitProfiledOpcode(op_get_direct_pname);
         instructions().append(kill(dst));
@@ -4632 +4632 @@
     if (!localRegister)
         return;
-
-    ASSERT(m_forInContextStack.last()->type() == ForInContext::IndexedForInContextType);
-    static_cast<IndexedForInContext&>(m_forInContextStack.last().get()).finalize(*this);
+    m_forInContextStack.last()->asIndexedForInContext().finalize(*this);
     m_forInContextStack.removeLast();
 }
@@ -4744 +4742 @@
     if (!localRegister)
         return;
-    ASSERT(m_forInContextStack.last()->type() == ForInContext::StructureForInContextType);
-    static_cast<StructureForInContext&>(m_forInContextStack.last().get()).finalize(*this);
+    m_forInContextStack.last()->asStructureForInContext().finalize(*this);
     m_forInContextStack.removeLast();
 }

trunk/Source/JavaScriptCore/bytecompiler/BytecodeGenerator.h

@@ -57 +57 @@
     class JSImmutableButterfly;
     class Identifier;
+    class IndexedForInContext;
+    class StructureForInContext;
 
     enum ExpectedFunction {
@@ -181 +183 @@
         WTF_MAKE_NONCOPYABLE(ForInContext);
     public:
-        ForInContext(RegisterID* localRegister)
-            : m_localRegister(localRegister)
-            , m_isValid(true)
-        {
-        }
-
-        virtual ~ForInContext()
-        {
-        }
+        virtual ~ForInContext() = default;
 
         bool isValid() const { return m_isValid; }
         void invalidate() { m_isValid = false; }
 
-        enum ForInContextType {
-            StructureForInContextType,
-            IndexedForInContextType
+        enum class Type : uint8_t {
+            IndexedForIn,
+            StructureForIn
         };
-        virtual ForInContextType type() const = 0;
+
+        Type type() const { return m_type; }
+        bool isIndexedForInContext() const { return m_type == Type::IndexedForIn; }
+        bool isStructureForInContext() const { return m_type == Type::StructureForIn; }
+
+        IndexedForInContext& asIndexedForInContext()
+        {
+            ASSERT(isIndexedForInContext());
+            return *reinterpret_cast<IndexedForInContext*>(this);
+        }
+
+        StructureForInContext& asStructureForInContext()
+        {
+            ASSERT(isStructureForInContext());
+            return *reinterpret_cast<StructureForInContext*>(this);
+        }
 
         RegisterID* local() const { return m_localRegister.get(); }
+
+    protected:
+        ForInContext(RegisterID* localRegister, Type type)
+            : m_localRegister(localRegister)
+            , m_type(type)
+        { }
 
     private:
         RefPtr<RegisterID> m_localRegister;
-        bool m_isValid;
+        bool m_isValid { true };
+        Type m_type;
     };
 
@@ -212 +228 @@
 
         StructureForInContext(RegisterID* localRegister, RegisterID* indexRegister, RegisterID* propertyRegister, RegisterID* enumeratorRegister)
-            : ForInContext(localRegister)
+            : ForInContext(localRegister, Type::StructureForIn)
             , m_indexRegister(indexRegister)
             , m_propertyRegister(propertyRegister)
             , m_enumeratorRegister(enumeratorRegister)
         {
-        }
-
-        ForInContextType type() const override
-        {
-            return StructureForInContextType;
         }
 
@@ -245 +256 @@
     public:
         IndexedForInContext(RegisterID* localRegister, RegisterID* indexRegister)
-            : ForInContext(localRegister)
+            : ForInContext(localRegister, Type::IndexedForIn)
             , m_indexRegister(indexRegister)
         {
-        }
-
-        ForInContextType type() const override
-        {
-            return IndexedForInContextType;
         }