[webkit-reviews] review denied: [Bug 90095] JSC: add infrastructure for appending comments to generated bytecode : [Attachment 149842] Rev 1: removed a tab that snuck in.

[bugzilla-daemon at webkit.org]

Geoffrey Garen <ggaren at apple.com> has denied Mark Lam <mark.lam at apple.com>'s
request for review:
Bug 90095: JSC: add infrastructure for appending comments to generated bytecode
https://bugs.webkit.org/show_bug.cgi?id=90095

Attachment 149842: Rev 1: removed a tab that snuck in.
https://bugs.webkit.org/attachment.cgi?id=149842&action=review

------- Additional Comments from Geoffrey Garen <ggaren at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=149842&action=review


Design looks fine, but this patch could use some small tweaks.

> Source/JavaScriptCore/ChangeLog:10
> +	   development purposes.  It should not be enable for product builds.

Typo: "enable" should be "enabled".

> Source/JavaScriptCore/bytecode/CodeBlock.cpp:99
> +#ifdef USE_BYTECODE_COMMENTS

The WebKit style for this kind of #ifdef is the "ENABLE()" macro.

To turn the feature on: #define ENABLE_BYTECODE_COMMENTS 1

To test the feature: #if ENABLE(BYTECODE_COMMENTS)

> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:691
> +// Record a comment in the CodeBlock's comments list for the current opcode
> +// that is about to be emitted.

Better to put a comment like this in the header, by the function declaration.

> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:702
> +// Register a comment to be associated with the next opcode that will be
emitted.

Ditto.