[webkit-reviews] review granted: [Bug 206986] [JSC] Remove unnecessary allocations in BytecodeBasicBlock : [Attachment 389229] Patch

[bugzilla-daemon at webkit.org]

Mark Lam <mark.lam at apple.com> has granted Yusuke Suzuki <ysuzuki at apple.com>'s
request for review:
Bug 206986: [JSC] Remove unnecessary allocations in BytecodeBasicBlock
https://bugs.webkit.org/show_bug.cgi?id=206986

Attachment 389229: Patch

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




--- Comment #2 from Mark Lam <mark.lam at apple.com> ---
Comment on attachment 389229
  --> https://bugs.webkit.org/attachment.cgi?id=389229
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=389229&action=review

r=me with comments.

> Source/JavaScriptCore/bytecode/BytecodeBasicBlock.cpp:38
> +inline BytecodeBasicBlock::BytecodeBasicBlock(const InstructionStream::Ref&
instruction, unsigned index)

I recommend renaming index to blockIndex to make it clear that it's not the
instruction index.

> Source/JavaScriptCore/bytecode/BytecodeBasicBlock.cpp:46
> +inline
BytecodeBasicBlock::BytecodeBasicBlock(BytecodeBasicBlock::SpecialBlockType
blockType, unsigned index)

Ditto.

> Source/JavaScriptCore/bytecode/BytecodeBasicBlock.h:53
> +    BytecodeBasicBlock(const InstructionStream::Ref&, unsigned);
> +    BytecodeBasicBlock(SpecialBlockType, unsigned);

I recommend naming the unsigned params as blockIndex to document their intended
use.
I also recommend explicitly declaring these as inline.	That will help the
compiler error out immediately if any clients try to use these, rather than
waiting for the linker to find the error.

Alternatively, make these private and declare the Vector template a friend (is
that possible?) since they are only intended to be constructed in a Vector.

> Source/JavaScriptCore/bytecode/BytecodeBasicBlock.h:71
> +    explicit operator bool() const { return true; }

Why do we need this?

My understanding is that this is only needed by the Vector implementation, and
that it should always return true because we'll never have an "empty"
BytecodeBasicBlock in the Vectors that we use it in.  If this is correct, can
you add a comment to document that?  I this logic is not necessarily obvious.

> Source/JavaScriptCore/bytecode/BytecodeBasicBlock.h:82
> +    void addLength(unsigned length);

nit: the length param name seems obvious and therefore, is probably not needed.

> Source/JavaScriptCore/bytecode/Opcode.h:91
> +#define CALCULATE_MAX_LENGTH(id, length) length,

This appears to be unused.  Please remove.

> Source/JavaScriptCore/bytecode/Opcode.h:94
> +#undef CALCULATE_MAX_LENGTH

Ditto.	Please remove.