OS#16244108: Small refactor of Parser::CreateCallNode and Parser::CreateSuperCallNode #4804

IrinaYatsenko · 2018-03-09T23:31:11Z

https://microsoft.visualstudio.com/OS/_workitems/edit/16244108

The uninitialized field was introduced by #3917. The only read of the field is in EmitArgList, and if it ends up "true" instead of the default "false" an extra defensive load will be emitted for the constructor parameters so not a security/correctness concern.

MSLaguana · 2018-03-09T23:40:55Z

lib/Parser/Parse.cpp

+{
+    pnode->nop = nop;
+
+    // make Parser::Init public?


Alternately, make this Parser::InitCallNode and have it delegate this first part to Parser::InitNode? #Resolved

Yes, could do that too. What is our philosophy on free functions versus members? :) #Resolved

Judging by this file, I think that static class functions would fit in well. I also noticed something similar to what you are doing here:

https://github.com/irinayat-MS/ChakraCore/blob/97412c4447799ed3931f85caabdcaa989564ae33/lib/Parser/Parse.cpp#L713-L718

Perhaps instead of allocating space explicitly in Create[Super]CallNode, it would be cleaner to StaticCreateNodeT<kcbPn[Super]Call>(m_nodeAllocator, ...) and then InitCallNode on the result of that? #Resolved

As we've been generally making a push to move the ParseNodes into a real class hierarchy, I am kind of a fan of adding a method like ParseNodeCall::Init which would do this stuff and delegate up to an ParseNode::Init for the base class stuff. Or just continue to rely on InitNode for that. Could use pnode->AsParseNodeCall()->Init or something along those lines. Regardless, I prefer to avoid the duplication of initializing these members in two places. #Resolved

To be clear, this is fine though. Just been thinking about cleanups in this area since Curtis refactored the ParseNodes recently. #Resolved

I find private static methods on a class somewhat weird. Made a step towards class hierarchy with type dependent "Init" methods. Is that what you had in mind?

In reply to: 173598566 [](ancestors = 173598566)

Yes, that's what I was thinking about. 👍 #Resolved

boingoing

👍

IrinaYatsenko · 2018-03-10T00:52:43Z

lib/Parser/Parse.h

@@ -380,7 +377,6 @@ class Parser
    ParseNodePtr CreateIntNodeWithScanner(int32 lw);
    ParseNodePtr CreateProgNodeWithScanner(bool isModuleSource);

-    static void InitNode(OpCode nop,ParseNodePtr pnode);
    static void InitBlockNode(ParseNodePtr pnode, int blockId, PnodeBlockType blockType);



Should I go through the exercise of replacing this static method with an instance Init as well? #Resolved

If you're feeling motivated. #Resolved

digitalinfinity · 2018-03-10T03:59:18Z

lib/Parser/ptree.cpp

@@ -4,6 +4,20 @@
 //-------------------------------------------------------------------------------------------------------
 #include "ParserPch.h"

+void ParseNode::Init(OpCode nop, charcount_t ichMin, charcount_t ichLim)


Dumb question, and maybe @boingoing knows the answer- why not just make this a constructor instead of an explicitly named Init function? #WontFix

and then use placement new at the call sites? Is that what you mean?

In reply to: 173610992 [](ancestors = 173610992)

the placement new is wrapped by AllocatorNew

In reply to: 173858893 [](ancestors = 173858893,173610992)

the inits could be replaced by constructors + AllocatorNew at the callsite, however the constructors have to be propagated through the inheritance hierarchy and would hide default ones. I think we could wait with this till ParseNode* classes get more flesh and then we should consistently provide all of them with constructors (and remove Init methods if any).

In reply to: 173897962 [](ancestors = 173897962,173858893,173610992)

digitalinfinity

MSLaguana · 2018-03-12T18:33:04Z

It looks like this has had some semantic changes; the "hello world" test we run on release builds is failing with

11:23:56 SyntaxError: Declaration outside statement context
11:23:56 	at code (hello.js:6:13)
``` #Pending

…ode.

…ateCallNode and Parser::CreateSuperCallNode Merge pull request #4804 from irinayat-MS:ParseNodeCall https://microsoft.visualstudio.com/OS/_workitems/edit/16244108 The uninitialized field was introduced by #3917. The only read of the field is in EmitArgList, and if it ends up "true" instead of the default "false" an extra defensive load will be emitted for the constructor parameters so not a security/correctness concern.

IrinaYatsenko requested a review from boingoing March 9, 2018 23:31

MSLaguana reviewed Mar 9, 2018

View reviewed changes

MSLaguana approved these changes Mar 9, 2018

View reviewed changes

boingoing approved these changes Mar 10, 2018

View reviewed changes

IrinaYatsenko commented Mar 10, 2018

View reviewed changes

digitalinfinity reviewed Mar 10, 2018

View reviewed changes

digitalinfinity approved these changes Mar 10, 2018

View reviewed changes

Small refactor of Parser::CreateCallNode and Parser::CreateSuperCallN…

bb68776

…ode.

IrinaYatsenko force-pushed the ParseNodeCall branch from 79e08df to bb68776 Compare March 12, 2018 20:31

chakrabot merged commit bb68776 into chakra-core:master Mar 12, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

OS#16244108: Small refactor of Parser::CreateCallNode and Parser::CreateSuperCallNode #4804

OS#16244108: Small refactor of Parser::CreateCallNode and Parser::CreateSuperCallNode #4804

IrinaYatsenko commented Mar 9, 2018

MSLaguana Mar 9, 2018 •

edited by IrinaYatsenko

Loading

IrinaYatsenko Mar 9, 2018 •

edited

Loading

MSLaguana Mar 9, 2018 •

edited by IrinaYatsenko

Loading

boingoing Mar 10, 2018 •

edited by IrinaYatsenko

Loading

boingoing Mar 10, 2018 •

edited by IrinaYatsenko

Loading

IrinaYatsenko Mar 10, 2018

boingoing Mar 10, 2018 •

edited by IrinaYatsenko

Loading

boingoing left a comment

IrinaYatsenko Mar 10, 2018 •

edited

Loading

boingoing Mar 10, 2018 •

edited by IrinaYatsenko

Loading

digitalinfinity Mar 10, 2018 •

edited by IrinaYatsenko

Loading

IrinaYatsenko Mar 12, 2018

IrinaYatsenko Mar 12, 2018

IrinaYatsenko Mar 12, 2018

digitalinfinity left a comment

MSLaguana commented Mar 12, 2018 •

edited by IrinaYatsenko

Loading

OS#16244108: Small refactor of Parser::CreateCallNode and Parser::CreateSuperCallNode #4804

OS#16244108: Small refactor of Parser::CreateCallNode and Parser::CreateSuperCallNode #4804

Conversation

IrinaYatsenko commented Mar 9, 2018

MSLaguana Mar 9, 2018 • edited by IrinaYatsenko Loading

Choose a reason for hiding this comment

IrinaYatsenko Mar 9, 2018 • edited Loading

Choose a reason for hiding this comment

MSLaguana Mar 9, 2018 • edited by IrinaYatsenko Loading

Choose a reason for hiding this comment

boingoing Mar 10, 2018 • edited by IrinaYatsenko Loading

Choose a reason for hiding this comment

boingoing Mar 10, 2018 • edited by IrinaYatsenko Loading

Choose a reason for hiding this comment

IrinaYatsenko Mar 10, 2018

Choose a reason for hiding this comment

boingoing Mar 10, 2018 • edited by IrinaYatsenko Loading

Choose a reason for hiding this comment

boingoing left a comment

Choose a reason for hiding this comment

IrinaYatsenko Mar 10, 2018 • edited Loading

Choose a reason for hiding this comment

boingoing Mar 10, 2018 • edited by IrinaYatsenko Loading

Choose a reason for hiding this comment

digitalinfinity Mar 10, 2018 • edited by IrinaYatsenko Loading

Choose a reason for hiding this comment

IrinaYatsenko Mar 12, 2018

Choose a reason for hiding this comment

IrinaYatsenko Mar 12, 2018

Choose a reason for hiding this comment

IrinaYatsenko Mar 12, 2018

Choose a reason for hiding this comment

digitalinfinity left a comment

Choose a reason for hiding this comment

MSLaguana commented Mar 12, 2018 • edited by IrinaYatsenko Loading

MSLaguana Mar 9, 2018 •

edited by IrinaYatsenko

Loading

IrinaYatsenko Mar 9, 2018 •

edited

Loading

MSLaguana Mar 9, 2018 •

edited by IrinaYatsenko

Loading

boingoing Mar 10, 2018 •

edited by IrinaYatsenko

Loading

boingoing Mar 10, 2018 •

edited by IrinaYatsenko

Loading

boingoing Mar 10, 2018 •

edited by IrinaYatsenko

Loading

IrinaYatsenko Mar 10, 2018 •

edited

Loading

boingoing Mar 10, 2018 •

edited by IrinaYatsenko

Loading

digitalinfinity Mar 10, 2018 •

edited by IrinaYatsenko

Loading

MSLaguana commented Mar 12, 2018 •

edited by IrinaYatsenko

Loading