[webkit-reviews] review granted: [Bug 203276] BytecodeIndex should be a proper C++ class : [Attachment 381618] Patch

[bugzilla-daemon at webkit.org]

Mark Lam <mark.lam at apple.com> has granted Keith Miller
<keith_miller at apple.com>'s request for review:
Bug 203276: BytecodeIndex should be a proper C++ class
https://bugs.webkit.org/show_bug.cgi?id=203276

Attachment 381618: Patch

https://bugs.webkit.org/attachment.cgi?id=381618&action=review




--- Comment #5 from Mark Lam <mark.lam at apple.com> ---
Comment on attachment 381618
  --> https://bugs.webkit.org/attachment.cgi?id=381618
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)?