Bug 148665 – [ES6] Recognize calls in tail position

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Basile Clement]

...

[Basile Clement]

*** Bug 146481 has been marked as a duplicate of this bug. ***

[Basile Clement]

Created attachment 260345 [details]
Patch

[Basile Clement]

Created attachment 260346 [details]
Patch

[Saam Barati]

Comment on attachment 260346 [details]
Patch

r=me

[Geoffrey Garen]

Comment on attachment 260346 [details]
Patch

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

> Source/JavaScriptCore/parser/Nodes.h:85
> +    enum TailCallMode {
> +        CanHaveTailCalls,
> +        NoTailCalls,
> +    };

Please use "enum class".

"CanHaveTailCalls" seems to wishy-washy. This is the value we use to indicate that we are in tail position, so all calls will be tail calls, right? If so, I would call this "enum class TailCallMode { DoTailCall, DoNotTailCall }" or "enum class TailPosition { InTailPosition, NotInTailPosition }".

It would be nice if we stored this state on the bytecode generator, rather than passing it through to each emitBytecode function. There's a lot of boilerplate for passing this value through, which we don't need.

[Basile Clement]

(In reply to comment #5)
> Comment on attachment 260346 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=260346&action=review
> 
> > Source/JavaScriptCore/parser/Nodes.h:85
> > +    enum TailCallMode {
> > +        CanHaveTailCalls,
> > +        NoTailCalls,
> > +    };
> 
> Please use "enum class".
> 
> "CanHaveTailCalls" seems to wishy-washy. This is the value we use to
> indicate that we are in tail position, so all calls will be tail calls,
> right? If so, I would call this "enum class TailCallMode { DoTailCall,
> DoNotTailCall }" or "enum class TailPosition { InTailPosition,
> NotInTailPosition }".

When said this way, it makes sense. I couldn't find a good name because I got stuck thinking that if we don't have an actual call, then we are not doing a tail call, and I kind of got confused by this. I like the TailPosition enum class.

> 
> It would be nice if we stored this state on the bytecode generator, rather
> than passing it through to each emitBytecode function. There's a lot of
> boilerplate for passing this value through, which we don't need.

Unless I am missing something, storing this on the bytecode generator would make us explicitely clear the "in tail position" state on all constructs that prevent tail calls. The reason I used the "function argument" style was that it allowed to explicitely pass that information for constructs that supports tail calls, and make the default to not have tail calls. This makes it simpler for AST edges that don't support tail calls (which is a bunch of them), and also makes it less error-prone to add new nodes. But maybe I am missing an implementation possibility here.

[Basile Clement]

*** Bug 146481 has been marked as a duplicate of this bug. ***

[Basile Clement]

Created attachment 260381 [details]
Patch

[Saam Barati]

Comment on attachment 260381 [details]
Patch

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

r=me with suggestion.

> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.h:68
> +    class SetForScope {

I think we should give this its own file in hopes of using it in other contexts.

[Basile Clement]

Created attachment 260552 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 260552 [details]
Patch for landing

Clearing flags on attachment: 260552

Committed r189336: <http://trac.webkit.org/changeset/189336>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.