Attachment 349792 Details for Bug 189626 – proposed patch.

[patch] proposed patch.

bug-189626.patch (text/plain), 7.88 KB, created by Mark Lam on 2018-09-14 13:13:19 PDT

(hide)

Description:

Filename:

MIME Type:

Creator: Mark Lam

Created: 2018-09-14 13:13:19 PDT

Size: 7.88 KB

patch

obsolete

>Index: Source/JavaScriptCore/ChangeLog
>===================================================================
>--- Source/JavaScriptCore/ChangeLog	(revision 236016)
>+++ Source/JavaScriptCore/ChangeLog	(working copy)
>@@ -1,3 +1,40 @@
>+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 NOBODY (OOPS!).
>+
>+        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>
> 
>         Web Inspector: Record actions performed on ImageBitmapRenderingContext
>Index: Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp
>===================================================================
>--- Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp	(revision 236015)
>+++ Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp	(working copy)
>@@ -2912,14 +2912,14 @@ RegisterID* BytecodeGenerator::emitGetBy
> 
>         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));
>         instructions().append(base->index());
>@@ -4631,9 +4631,7 @@ void BytecodeGenerator::popIndexedForInS
> {
>     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();
> }
> 
>@@ -4743,8 +4741,7 @@ void BytecodeGenerator::popStructureForI
> {
>     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();
> }
> 
>Index: Source/JavaScriptCore/bytecompiler/BytecodeGenerator.h
>===================================================================
>--- Source/JavaScriptCore/bytecompiler/BytecodeGenerator.h	(revision 236015)
>+++ Source/JavaScriptCore/bytecompiler/BytecodeGenerator.h	(working copy)
>@@ -56,6 +56,8 @@ namespace JSC {
> 
>     class JSImmutableButterfly;
>     class Identifier;
>+    class IndexedForInContext;
>+    class StructureForInContext;
> 
>     enum ExpectedFunction {
>         NoExpectedFunction,
>@@ -180,30 +182,44 @@ namespace JSC {
>         WTF_MAKE_FAST_ALLOCATED;
>         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;
>     };
> 
>     class StructureForInContext : public ForInContext {
>@@ -211,18 +227,13 @@ namespace JSC {
>         using GetInst = std::tuple<unsigned, int, UnlinkedValueProfile>;
> 
>         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;
>-        }
>-
>         RegisterID* index() const { return m_indexRegister.get(); }
>         RegisterID* property() const { return m_propertyRegister.get(); }
>         RegisterID* enumerator() const { return m_enumeratorRegister.get(); }
>@@ -244,16 +255,11 @@ namespace JSC {
>     class IndexedForInContext : public ForInContext {
>     public:
>         IndexedForInContext(RegisterID* localRegister, RegisterID* indexRegister)
>-            : ForInContext(localRegister)
>+            : ForInContext(localRegister, Type::IndexedForIn)
>             , m_indexRegister(indexRegister)
>         {
>         }
> 
>-        ForInContextType type() const override
>-        {
>-            return IndexedForInContextType;
>-        }
>-
>         RegisterID* index() const { return m_indexRegister.get(); }
> 
>         void finalize(BytecodeGenerator&);

Flags:

keith_miller: review+

Actions: View | Formatted Diff | Diff

Attachments on bug 189626: 349792