RESOLVED FIXED 147051
Add support for new.target.
https://bugs.webkit.org/show_bug.cgi?id=147051
Summary Add support for new.target.
Keith Miller
Reported 2015-07-17 14:42:58 PDT
ES6 specifies a new expression new.target that is used to get the target of a "new" call, this is analogus to a this for constructors. For example, if someone called "new function Foo() { print(new.target) }" the resulting output would be the same as "print(Foo.toString())".
Attachments
Patch (11.37 KB, patch)
2015-07-17 14:54 PDT, Keith Miller
no flags
Patch (11.35 KB, patch)
2015-07-17 14:59 PDT, Keith Miller
ysuzuki: review-
Patch (11.49 KB, patch)
2015-07-20 10:34 PDT, Keith Miller
no flags
Patch (11.49 KB, patch)
2015-07-20 10:37 PDT, Keith Miller
no flags
Patch (11.52 KB, patch)
2015-07-20 10:45 PDT, Keith Miller
no flags
Keith Miller
Comment 1 2015-07-17 14:54:03 PDT
WebKit Commit Bot
Comment 2 2015-07-17 14:55:56 PDT
Attachment 256992 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/ChangeLog:1: ChangeLog entry has no bug number [changelog/bugnumber] [5] ERROR: Source/JavaScriptCore/parser/Parser.cpp:2918: Missing space before ( in if( [whitespace/parens] [5] Total errors found: 2 in 10 files If any of these errors are false positives, please file a bug against check-webkit-style.
Keith Miller
Comment 3 2015-07-17 14:59:44 PDT
Created attachment 256994 [details] Patch Fixed a url issue.
Yusuke Suzuki
Comment 4 2015-07-18 14:57:08 PDT
Comment on attachment 256994 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=256994&action=review Looks nice! only few nits. > Source/JavaScriptCore/parser/Parser.cpp:2929 > + if (newCount && m_token.m_type == DOT) { Let's use `match(DOT)`. > Source/JavaScriptCore/parser/Parser.cpp:2931 > + const Identifier* ident = m_token.m_data.ident; Before extracting ident* from m_token, using `if (match(IDENT))` is preferable.
Saam Barati
Comment 5 2015-07-19 12:03:03 PDT
Comment on attachment 256994 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=256994&action=review > Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:190 > +RegisterID* TargetNode::emitBytecode(BytecodeGenerator& generator, RegisterID* dst) Nit: Can we call this NewTargetNode?
Keith Miller
Comment 6 2015-07-20 10:34:59 PDT
Created attachment 257097 [details] Patch Made the changes recommended. Also, improved the error message if the user puts the wrong identifier after "new."
Keith Miller
Comment 7 2015-07-20 10:37:28 PDT
Created attachment 257098 [details] Patch Had an extra period in the error message.
Keith Miller
Comment 8 2015-07-20 10:45:40 PDT
Created attachment 257099 [details] Patch I realized that if I was going the node to NewTargetNode I should also change the expression generator to newTargetExpr.
Saam Barati
Comment 9 2015-07-20 12:00:37 PDT
Comment on attachment 257099 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=257099&action=review > Source/JavaScriptCore/parser/ASTBuilder.h:183 > + ExpressionNode* newTargetExpr(const JSTokenLocation location) small nit: We usually try to name things that create AST nodes "createX". So here, "createNewTargetNode" or "createNewTargetExpression".
Keith Miller
Comment 10 2015-07-20 12:21:41 PDT
Comment on attachment 257099 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=257099&action=review >> Source/JavaScriptCore/parser/ASTBuilder.h:183 >> + ExpressionNode* newTargetExpr(const JSTokenLocation location) > > small nit: We usually try to name things that create AST nodes "createX". > So here, "createNewTargetNode" or "createNewTargetExpression". Is there a reason why thisExpr and superExpr don't follow this rule? I named my function "newTargetExpr" since new.target is similar to this but for constructors, so I was trying to emulate those functions.
Yusuke Suzuki
Comment 11 2015-07-20 17:32:46 PDT
Comment on attachment 257099 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=257099&action=review >>> Source/JavaScriptCore/parser/ASTBuilder.h:183 >>> + ExpressionNode* newTargetExpr(const JSTokenLocation location) >> >> small nit: We usually try to name things that create AST nodes "createX". >> So here, "createNewTargetNode" or "createNewTargetExpression". > > Is there a reason why thisExpr and superExpr don't follow this rule? I named my function "newTargetExpr" since new.target is similar to this but for constructors, so I was trying to emulate those functions. Ah, I guess they are just overlooked ones. Could you rename them to createThisExpr, createSuperExpr etc. at the same time?
Keith Miller
Comment 12 2015-07-21 09:50:33 PDT
Comment on attachment 257099 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=257099&action=review >>>> Source/JavaScriptCore/parser/ASTBuilder.h:183 >>>> + ExpressionNode* newTargetExpr(const JSTokenLocation location) >>> >>> small nit: We usually try to name things that create AST nodes "createX". >>> So here, "createNewTargetNode" or "createNewTargetExpression". >> >> Is there a reason why thisExpr and superExpr don't follow this rule? I named my function "newTargetExpr" since new.target is similar to this but for constructors, so I was trying to emulate those functions. > > Ah, I guess they are just overlooked ones. Could you rename them to createThisExpr, createSuperExpr etc. at the same time? I'm not a huge fan of mixing new code with refactors, in case things get rolled out. I'll throw another patch up with that fix.
Yusuke Suzuki
Comment 13 2015-07-21 10:29:02 PDT
Comment on attachment 257099 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=257099&action=review r+ >>>>> Source/JavaScriptCore/parser/ASTBuilder.h:183 >>>>> + ExpressionNode* newTargetExpr(const JSTokenLocation location) >>>> >>>> small nit: We usually try to name things that create AST nodes "createX". >>>> So here, "createNewTargetNode" or "createNewTargetExpression". >>> >>> Is there a reason why thisExpr and superExpr don't follow this rule? I named my function "newTargetExpr" since new.target is similar to this but for constructors, so I was trying to emulate those functions. >> >> Ah, I guess they are just overlooked ones. Could you rename them to createThisExpr, createSuperExpr etc. at the same time? > > I'm not a huge fan of mixing new code with refactors, in case things get rolled out. I'll throw another patch up with that fix. Make sense.
WebKit Commit Bot
Comment 14 2015-07-21 11:20:00 PDT
Comment on attachment 257099 [details] Patch Clearing flags on attachment: 257099 Committed r187108: <http://trac.webkit.org/changeset/187108>
WebKit Commit Bot
Comment 15 2015-07-21 11:20:04 PDT
All reviewed patches have been landed. Closing bug.
Keith Miller
Comment 16 2015-07-21 13:23:50 PDT
Note You need to log in before you can comment on or make changes to this bug.