[Webkit-unassigned] [Bug 140855] [ES6] Implement ES6 arrow function syntax

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=140855

--- Comment #50 from GSkachkov <gskachkov at gmail.com> ---
(In reply to comment #47)
> Comment on attachment 249664 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=249664&action=review
> 
> Looking pretty good. Some comments below:
> 
> > Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:1752
> > +    RegisterID* value = emitThisNodeBytecode(r0, n->position());
> 
> I don't think this is doing exactly what you want here.
> r0 here is the destination of this function expression, and you will 
> move lexical 'this' into this destination, then, you will override that
> with the function expression. I don't think this is the intention. I'm
> not exactly sure of the consequences of this are, but it may be easier to
> get rid of the ::emitThisNodeBytecode function, and just do this here:
> ```RegiserID* value = emitMove(newTemporary(), thisRegister()),``
> or, you may even just be able to do:
> ```instructions().append(thisRegister()->index())```
> 
> > Source/JavaScriptCore/jit/JITOpcodes.cpp:985
> > +    emitGetVirtualRegister(currentInstruction[4].u.operand, regT1);
> 
> I don't think you want to load 'this' after the above branch because
> you need it at the branch destination. (same for 32-bit version below).
> 
> > Source/JavaScriptCore/jit/JITOpcodes.cpp:995
> > +    FunctionExecutable* funcExpr = m_codeBlock->functionExpr(currentInstruction[3].u.operand);
> 
> Nit: I would call this 'executable' or 'function' or something other than
> 'funcExpr'.
> 
> > Source/JavaScriptCore/jit/JITOperations.cpp:951
> > +    return JSValue::encode(JSBoundFunction::create(vm, globalObject, func, JSValue::decode(thisValue), boundArgs, 0, ""));
> 
> Maybe we should give this a name here instead of ""?
> We should investigate this.
> 
> Also, is 0 the correct thing to pass here?
> I'm not really sure what that does.
> 
> > Source/JavaScriptCore/llint/LLIntSlowPaths.cpp:1013
> > +    LLINT_RETURN(JSBoundFunction::create(vm, globalObject, func, thisValue, boundArgs, 0, ""));
> 
> ditto.
> 
> Also, maybe it's worth having some kind of helper function here 
> because this function is so similar to the above.
> 
> > Source/JavaScriptCore/parser/Parser.cpp:1538
> > +{
> 
> I still think this function has too much code in common with
> ::parseFunctionInfo,
> we should abstract away the common bits.
> 
> > Source/JavaScriptCore/parser/Parser.cpp:2080
> > +#endif
> 
> Nit: I think that can just be one contiguous region, not need for #endif
> then #if
> 
> > Source/JavaScriptCore/parser/Parser.h:560
> > +    const SourceProviderCacheItem* findCachedFunctionInfo(int openBracePos)
> 
> Nit: whitespace
> 
> > Source/JavaScriptCore/parser/Parser.h:568
> > +    void didFinishParsing(SourceElements*, DeclarationStacks::VarStack&,
> 
> ditto
> 
> > Source/JavaScriptCore/tests/stress/arrowfunction.js:1
> > +var result;
> 
> I think we need some tests for lexical 'this'.

I really appreciate your feedback. It allow to fix several failing tests. 
I've tried to fix most of the comments with new patch. I'll provide more detail for each your comments after checking the build by CI.

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.webkit.org/pipermail/webkit-unassigned/attachments/20150331/070a1f29/attachment-0002.html>