...
*** Bug 146481 has been marked as a duplicate of this bug. ***
Created attachment 260345 [details] Patch
Created attachment 260346 [details] Patch
Comment on attachment 260346 [details] Patch r=me
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.
(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.
Created attachment 260381 [details] Patch
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.
Created attachment 260552 [details] Patch for landing
Comment on attachment 260552 [details] Patch for landing Clearing flags on attachment: 260552 Committed r189336: <http://trac.webkit.org/changeset/189336>
All reviewed patches have been landed. Closing bug.