BytecodeIndex should be a proper C++ class
Created attachment 381615 [details] Patch
Created attachment 381616 [details] Patch
Created attachment 381617 [details] Patch
Created attachment 381618 [details] Patch
Comment on attachment 381618 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=381618&action=review r=me with fixes. > Source/JavaScriptCore/bytecode/BytecodeIndex.h:53 > + explicit operator bool() const { return m_offset != std::numeric_limits<uint32_t>::max(); } Can you use a "static constexpr InvalidOffset = std::numeric_limits<uint32_t>::max();" and use InvalidOffset here instead? I think it will read better. > Source/JavaScriptCore/bytecode/BytecodeIndex.h:66 > + uint32_t m_offset { std::numeric_limits<uint32_t>::max() }; Ditto. Use InvalidOffset here. > Source/JavaScriptCore/bytecode/InstructionStream.h:80 > + inline Offset offset() const { return m_index; } Rename m_index to m_offset? This cross-mixed naming is going to be a source of confusion. You can do this in a follow up patch. > Source/JavaScriptCore/bytecode/LazyOperandValueProfile.h:48 > - : m_bytecodeOffset(0) // 0 = empty value > + : m_bytecodeIndex(0) // 0 = empty value > , m_operand(VirtualRegister()) // not a valid operand index in our current scheme > { > } > > LazyOperandValueProfileKey(WTF::HashTableDeletedValueType) > - : m_bytecodeOffset(1) // 1 = deleted value > + : m_bytecodeIndex(1) // 1 = deleted value These feels like they deserve a comment as to why you can't use the default empty and deleted value instead of 0 and 1. I presume the default values will cause problems? The comment will prevent someone naive (like myself) coming along in the future and changing them thus. > Source/JavaScriptCore/dfg/DFGOSREntrypointCreationPhase.cpp:55 > + RELEASE_ASSERT(bytecodeIndex.offset()); Shouldn't this be RELEASE_ASSERT(bytecodeIndex)?
Created attachment 381622 [details] Patch for landing
Comment on attachment 381618 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=381618&action=review >> Source/JavaScriptCore/bytecode/BytecodeIndex.h:53 >> + explicit operator bool() const { return m_offset != std::numeric_limits<uint32_t>::max(); } > > Can you use a "static constexpr InvalidOffset = std::numeric_limits<uint32_t>::max();" and use InvalidOffset here instead? I think it will read better. Done. >> Source/JavaScriptCore/bytecode/InstructionStream.h:80 >> + inline Offset offset() const { return m_index; } > > Rename m_index to m_offset? This cross-mixed naming is going to be a source of confusion. You can do this in a follow up patch. Sure. >> Source/JavaScriptCore/bytecode/LazyOperandValueProfile.h:48 >> + : m_bytecodeIndex(1) // 1 = deleted value > > These feels like they deserve a comment as to why you can't use the default empty and deleted value instead of 0 and 1. I presume the default values will cause problems? The comment will prevent someone naive (like myself) coming along in the future and changing them thus. I think I just left it because that's how it was before. I don't think it will cause any problems. I can change it. >> Source/JavaScriptCore/dfg/DFGOSREntrypointCreationPhase.cpp:55 >> + RELEASE_ASSERT(bytecodeIndex.offset()); > > Shouldn't this be RELEASE_ASSERT(bytecodeIndex)? No, the old line above now covers the != UINT_MAX case. This covers the bytecodeIndex.offset() == 0 case.
Comment on attachment 381618 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=381618&action=review >>> Source/JavaScriptCore/dfg/DFGOSREntrypointCreationPhase.cpp:55 >>> + RELEASE_ASSERT(bytecodeIndex.offset()); >> >> Shouldn't this be RELEASE_ASSERT(bytecodeIndex)? > > No, the old line above now covers the != UINT_MAX case. This covers the bytecodeIndex.offset() == 0 case. Oh, I see.
Committed r251468: <https://trac.webkit.org/changeset/251468>
<rdar://problem/56523782>
Committed r251471: <https://trac.webkit.org/changeset/251471>
(In reply to Yusuke Suzuki from comment #11) > Committed r251471: <https://trac.webkit.org/changeset/251471> Ah, thanks. Unfortunately, the WinCairo EWS wasn't working.
(In reply to Keith Miller from comment #12) > (In reply to Yusuke Suzuki from comment #11) > > Committed r251471: <https://trac.webkit.org/changeset/251471> > > Ah, thanks. Unfortunately, the WinCairo EWS wasn't working. And, maybe, we also need to update 32bit things (but EWS is not working, sad :(). 32bit had different implementation of CallSiteIndex. We should remove this difference I think.
Just an FYI, this change broke a bunch of our Windows testers, Zan fixed it in <https://trac.webkit.org/changeset/251534>