[webkit-reviews] review requested: [Bug 47107] Lazily create activation objects : [Attachment 69674] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has asked  for review:
Bug 47107: Lazily create activation objects
https://bugs.webkit.org/show_bug.cgi?id=47107

Attachment 69674: Patch
https://bugs.webkit.org/attachment.cgi?id=69674&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=69674&action=review

> JavaScriptCore/ChangeLog:13
> +	   This does make exception handling a little more dicey as

The word “dicey” here makes me nervous. I wish you had said it differently.

Is there enough test coverage?

> JavaScriptCore/bytecode/CodeBlock.h:433
> +	   void createActivation(CallFrame*);
>  #if ENABLE(INTERPRETER)

Why no blank line here? I would have had one.

> JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:1572
> +void BytecodeGenerator::createActivationIfNecessary()
> +{
> +    if (m_hasCreatedActivation)
> +	   return;
> +    if (!m_codeBlock->needsFullScopeChain())
> +	   return;
> +    emitOpcode(op_create_activation);
> +    instructions().append(m_activationRegister->index());
> +}

Should this function set m_hasCreatedActivation to true? If not, why not? It
might need a comment.

> JavaScriptCore/interpreter/Interpreter.cpp:131
> +    if (checkTopLevel && skip--) {

Same problem here that I spotted below.

> JavaScriptCore/interpreter/Interpreter.cpp:213
> +    if (checkTopLevel && skip--) {

Same problem here that I spotted below.

> JavaScriptCore/interpreter/Interpreter.cpp:2356
> +	   if (checkTopLevel && skip--) {

Same problem here that I spotted below.

> JavaScriptCore/jit/JITOpcodes.cpp:472
> +    if (checkTopLevel && skip--) {

Same problem here that I spotted below.

> JavaScriptCore/jit/JITOpcodes.cpp:494
> +    if (checkTopLevel && skip--) {

Same problem here that I spotted below.

> JavaScriptCore/jit/JITOpcodes.cpp:1590
> +    if (checkTopLevel && skip--) {

Same problem here that I spotted below.

> JavaScriptCore/jit/JITOpcodes32_64.cpp:614
> +    if (checkTopLevel && skip--) {

Same problem here that I spotted below.

> JavaScriptCore/jit/JITOpcodes32_64.cpp:643
> +    if (checkTopLevel && skip--) {

Same problem here that I spotted below.

> JavaScriptCore/jit/JITStubs.cpp:2697
> +    if (checkTopLevel && skip--) {

If skip is 0 this will set it to -1. I don’t think you want that.