Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

DYN-1541: Issues with code block function default arguments #9528

Merged
merged 38 commits into from
Mar 26, 2019
Merged

DYN-1541: Issues with code block function default arguments #9528

Show file tree
Hide file tree
Changes from 33 commits
Commits
Show all changes
38 commits
Select commit Hold shift + click to select a range
56dbd73
Update changes from librarie.js to fix QNTM-3710
ColinDayOrg Mar 22, 2018
eec64b9
Merge branch 'master' of https://github.com/DynamoDS/Dynamo
ColinDayOrg Mar 23, 2018
4362151
Merge branch 'master' of https://github.com/DynamoDS/Dynamo
ColinDayOrg Jul 19, 2018
75643c7
Merge branch 'master' of https://github.com/DynamoDS/Dynamo
ColinDayOrg Nov 2, 2018
6a07e6c
Merge branch 'master' of https://github.com/DynamoDS/Dynamo
ColinDayOrg Nov 19, 2018
38b8461
Merge branch 'master' of https://github.com/DynamoDS/Dynamo
ColinDayOrg Nov 21, 2018
44a00a8
Merge branch 'master' of https://github.com/DynamoDS/Dynamo
ColinDayOrg Nov 29, 2018
5c04a06
Merge branch 'master' of https://github.com/DynamoDS/Dynamo
ColinDayOrg Dec 7, 2018
a21fd6a
Merge branch 'master' of https://github.com/DynamoDS/Dynamo
ColinDayOrg Dec 10, 2018
4235133
Merge branch 'master' of https://github.com/DynamoDS/Dynamo
ColinDayOrg Dec 14, 2018
302b9dd
Merge branch 'master' of https://github.com/DynamoDS/Dynamo
ColinDayOrg Dec 14, 2018
6bf5548
Merge branch 'master' of https://github.com/DynamoDS/Dynamo
ColinDayOrg Dec 14, 2018
8d4511b
Merge branch 'master' of https://github.com/DynamoDS/Dynamo
ColinDayOrg Dec 21, 2018
8f9df0f
Merge branch 'master' of https://github.com/DynamoDS/Dynamo
ColinDayOrg Jan 8, 2019
e91ef30
Merge branch 'master' of https://github.com/DynamoDS/Dynamo
ColinDayOrg Jan 9, 2019
82373cc
Merge branch 'master' of https://github.com/DynamoDS/Dynamo
ColinDayOrg Jan 10, 2019
1434ba0
Merge branch 'master' of https://github.com/DynamoDS/Dynamo
ColinDayOrg Jan 10, 2019
706974c
Merge branch 'master' of https://github.com/DynamoDS/Dynamo
ColinDayOrg Jan 16, 2019
760ae95
Merge branch 'master' of https://github.com/DynamoDS/Dynamo
ColinDayOrg Feb 4, 2019
6ffd883
Merge branch 'master' of https://github.com/DynamoDS/Dynamo
ColinDayOrg Feb 15, 2019
5b1c073
Merge branch 'master' of https://github.com/DynamoDS/Dynamo
ColinDayOrg Feb 27, 2019
30f81af
Add some initial default argument tests
ColinDayOrg Feb 27, 2019
ccc23ef
Add one argument one default test
ColinDayOrg Feb 27, 2019
bab2f92
Comment out a purposely failing test
ColinDayOrg Feb 27, 2019
75e430a
Test multiple code blocks with default arguments
ColinDayOrg Feb 27, 2019
0bf25b4
Add a (commented out) test that fails due to indeterminate functions
ColinDayOrg Feb 27, 2019
4e7ed90
Add tests for deleting code block nodes with functions with default p…
ColinDayOrg Feb 27, 2019
e64b640
Remove deafult arguments to avoid a potential crash
ColinDayOrg Mar 8, 2019
072af66
Comment out code block deletion for testing
ColinDayOrg Mar 9, 2019
e4ae2c6
Uncomment test code
ColinDayOrg Mar 10, 2019
c06d338
remove emitting if statement node for associative inline conditionals
aparajit-pratap Mar 15, 2019
396e51d
obsolete property
aparajit-pratap Mar 15, 2019
1e81689
Merge branch 'DYN-1541' of https://github.com/ColinDayOrg/Dynamo into…
ColinDayOrg Mar 15, 2019
6ad7073
Add recordable test for node deletion
ColinDayOrg Mar 21, 2019
3dfe388
Restore public signature
ColinDayOrg Mar 21, 2019
fb22dd2
Remove obsolete test and update comment
ColinDayOrg Mar 21, 2019
042ec65
Use correct issue name in comment
ColinDayOrg Mar 21, 2019
57f2193
Fix typo
ColinDayOrg Mar 21, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
208 changes: 96 additions & 112 deletions src/Engine/ProtoAssociative/CodeGen.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3484,7 +3484,6 @@ private void EmitConstructorDefinitionNode(AssociativeNode node, ref ProtoCore.T
EmitBinaryExpressionNode(bNodeTemp, ref inferedType);
//duild an inline conditional node for statement: defaultarg = (temp == DefaultArgNode) ? defaultValue : temp;
InlineConditionalNode icNode = new InlineConditionalNode();
icNode.IsAutoGenerated = true;
BinaryExpressionNode cExprNode = new BinaryExpressionNode();
cExprNode.Optr = ProtoCore.DSASM.Operator.eq;
cExprNode.LeftNode = iNodeTemp;
Expand Down Expand Up @@ -3817,7 +3816,6 @@ private void EmitFunctionDefinitionNode(AssociativeNode node, ref ProtoCore.Type
EmitBinaryExpressionNode(bNodeTemp, ref inferedType, false, null, ProtoCore.CompilerDefinitions.SubCompilePass.UnboundIdentifier);
//duild an inline conditional node for statement: defaultarg = (temp == DefaultArgNode) ? defaultValue : temp;
InlineConditionalNode icNode = new InlineConditionalNode();
icNode.IsAutoGenerated = true;
BinaryExpressionNode cExprNode = new BinaryExpressionNode();
cExprNode.Optr = ProtoCore.DSASM.Operator.eq;
cExprNode.LeftNode = iNodeTemp;
Expand Down Expand Up @@ -4308,132 +4306,118 @@ private void EmitInlineConditionalNode(AssociativeNode node, ref ProtoCore.Type
}

InlineConditionalNode inlineConditionalNode = node as InlineConditionalNode;
// TODO: Jun, this 'if' condition needs to be removed as it was the old implementation - pratapa
if (inlineConditionalNode.IsAutoGenerated)
{
// Normal inline conditional

IfStatementNode ifNode = new IfStatementNode();
ifNode.ifExprNode = inlineConditionalNode.ConditionExpression;
List<AssociativeNode> ifBody = new List<AssociativeNode>();
List<AssociativeNode> elseBody = new List<AssociativeNode>();
ifBody.Add(inlineConditionalNode.TrueExpression);
elseBody.Add(inlineConditionalNode.FalseExpression);
ifNode.IfBody = ifBody;
ifNode.ElseBody = elseBody;
// CPS inline conditional

EmitIfStatementNode(ifNode, ref inferedType, graphNode);
}
else
{
// CPS inline conditional

FunctionCallNode inlineCall = new FunctionCallNode();
IdentifierNode identNode = new IdentifierNode();
identNode.Name = ProtoCore.DSASM.Constants.kInlineConditionalMethodName;
inlineCall.Function = identNode;
FunctionCallNode inlineCall = new FunctionCallNode();
IdentifierNode identNode = new IdentifierNode();
identNode.Name = ProtoCore.DSASM.Constants.kInlineConditionalMethodName;
inlineCall.Function = identNode;

DebugProperties.BreakpointOptions oldOptions = core.DebuggerProperties.breakOptions;
DebugProperties.BreakpointOptions newOptions = oldOptions;
newOptions |= DebugProperties.BreakpointOptions.EmitInlineConditionalBreakpoint;
core.DebuggerProperties.breakOptions = newOptions;
DebugProperties.BreakpointOptions oldOptions = core.DebuggerProperties.breakOptions;
DebugProperties.BreakpointOptions newOptions = oldOptions;
newOptions |= DebugProperties.BreakpointOptions.EmitInlineConditionalBreakpoint;
core.DebuggerProperties.breakOptions = newOptions;

core.DebuggerProperties.highlightRange = new ProtoCore.CodeModel.CodeRange
core.DebuggerProperties.highlightRange = new ProtoCore.CodeModel.CodeRange
{
StartInclusive = new ProtoCore.CodeModel.CodePoint
{
StartInclusive = new ProtoCore.CodeModel.CodePoint
{
LineNo = parentNode.line,
CharNo = parentNode.col
},
LineNo = parentNode.line,
CharNo = parentNode.col
},

EndExclusive = new ProtoCore.CodeModel.CodePoint
{
LineNo = parentNode.endLine,
CharNo = parentNode.endCol
}
};

// As SSA conversion is enabled, we have got the values of
// true and false branch, so it isn't necessary to create
// language blocks.
if (core.Options.GenerateSSA)
EndExclusive = new ProtoCore.CodeModel.CodePoint
{
inlineCall.FormalArguments.Add(inlineConditionalNode.ConditionExpression);
inlineCall.FormalArguments.Add(inlineConditionalNode.TrueExpression);
inlineCall.FormalArguments.Add(inlineConditionalNode.FalseExpression);
LineNo = parentNode.endLine,
CharNo = parentNode.endCol
}
else
{
// True condition language block
BinaryExpressionNode bExprTrue = AstFactory.BuildReturnStatement(inlineConditionalNode.TrueExpression);

LanguageBlockNode langblockT = new LanguageBlockNode();
int trueBlockId = Constants.kInvalidIndex;
langblockT.codeblock.Language = ProtoCore.Language.Associative;
core.AssocNode = bExprTrue;
core.InlineConditionalBodyGraphNodes.Push(new List<GraphNode>());
EmitDynamicLanguageBlockNode(langblockT, bExprTrue, ref inferedType, ref trueBlockId, graphNode, ProtoCore.CompilerDefinitions.SubCompilePass.None);
List<GraphNode> trueBodyNodes = core.InlineConditionalBodyGraphNodes.Pop();

// Append dependent nodes of the inline conditional
foreach (GraphNode gnode in trueBodyNodes)
foreach (GraphNode dNode in gnode.dependentList)
graphNode.PushDependent(dNode);

core.AssocNode = null;
DynamicBlockNode dynBlockT = new DynamicBlockNode(trueBlockId);

// False condition language block
BinaryExpressionNode bExprFalse = AstFactory.BuildReturnStatement(inlineConditionalNode.FalseExpression);
};

LanguageBlockNode langblockF = new LanguageBlockNode();
int falseBlockId = Constants.kInvalidIndex;
langblockF.codeblock.Language = ProtoCore.Language.Associative;
core.AssocNode = bExprFalse;
core.InlineConditionalBodyGraphNodes.Push(new List<GraphNode>());
EmitDynamicLanguageBlockNode(langblockF, bExprFalse, ref inferedType, ref falseBlockId, graphNode, ProtoCore.CompilerDefinitions.SubCompilePass.None);
List<GraphNode> falseBodyNodes = core.InlineConditionalBodyGraphNodes.Pop();
// As SSA conversion is enabled, we have got the values of
// true and false branch, so it isn't necessary to create
// language blocks.
if (core.Options.GenerateSSA)
{
inlineCall.FormalArguments.Add(inlineConditionalNode.ConditionExpression);
inlineCall.FormalArguments.Add(inlineConditionalNode.TrueExpression);
inlineCall.FormalArguments.Add(inlineConditionalNode.FalseExpression);
}
else
{
// True condition language block
BinaryExpressionNode bExprTrue = AstFactory.BuildReturnStatement(inlineConditionalNode.TrueExpression);

LanguageBlockNode langblockT = new LanguageBlockNode();
int trueBlockId = Constants.kInvalidIndex;
langblockT.codeblock.Language = ProtoCore.Language.Associative;
core.AssocNode = bExprTrue;
core.InlineConditionalBodyGraphNodes.Push(new List<GraphNode>());
EmitDynamicLanguageBlockNode(langblockT, bExprTrue, ref inferedType, ref trueBlockId, graphNode,
ProtoCore.CompilerDefinitions.SubCompilePass.None);
List<GraphNode> trueBodyNodes = core.InlineConditionalBodyGraphNodes.Pop();

// Append dependent nodes of the inline conditional
foreach (GraphNode gnode in trueBodyNodes)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

find this style with no braces kind of difficult to read / prone to future errors - what do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I actually find the requirement to have the extra braces unnecessary and creates code that is more unreadable (mostly due to adding unnecessary code and code lines), but the bracing wars is a long running issue across multiple companies, I really have no dog in this race. That being said, these changes were made by Aparajit, I can change them however you would like.

foreach (GraphNode dNode in gnode.dependentList)
graphNode.PushDependent(dNode);

// Append dependent nodes of the inline conditional
foreach (GraphNode gnode in falseBodyNodes)
foreach (GraphNode dNode in gnode.dependentList)
graphNode.PushDependent(dNode);
core.AssocNode = null;
DynamicBlockNode dynBlockT = new DynamicBlockNode(trueBlockId);

// False condition language block
BinaryExpressionNode bExprFalse =
AstFactory.BuildReturnStatement(inlineConditionalNode.FalseExpression);

LanguageBlockNode langblockF = new LanguageBlockNode();
int falseBlockId = Constants.kInvalidIndex;
langblockF.codeblock.Language = ProtoCore.Language.Associative;
core.AssocNode = bExprFalse;
core.InlineConditionalBodyGraphNodes.Push(new List<GraphNode>());
EmitDynamicLanguageBlockNode(langblockF, bExprFalse, ref inferedType, ref falseBlockId, graphNode,
ProtoCore.CompilerDefinitions.SubCompilePass.None);
List<GraphNode> falseBodyNodes = core.InlineConditionalBodyGraphNodes.Pop();

// Append dependent nodes of the inline conditional
foreach (GraphNode gnode in falseBodyNodes)
foreach (GraphNode dNode in gnode.dependentList)
graphNode.PushDependent(dNode);

core.AssocNode = null;
DynamicBlockNode dynBlockF = new DynamicBlockNode(falseBlockId);
core.AssocNode = null;
DynamicBlockNode dynBlockF = new DynamicBlockNode(falseBlockId);

inlineCall.FormalArguments.Add(inlineConditionalNode.ConditionExpression);
inlineCall.FormalArguments.Add(dynBlockT);
inlineCall.FormalArguments.Add(dynBlockF);
}
inlineCall.FormalArguments.Add(inlineConditionalNode.ConditionExpression);
inlineCall.FormalArguments.Add(dynBlockT);
inlineCall.FormalArguments.Add(dynBlockF);
}

core.DebuggerProperties.breakOptions = oldOptions;
core.DebuggerProperties.highlightRange = new ProtoCore.CodeModel.CodeRange
core.DebuggerProperties.breakOptions = oldOptions;
core.DebuggerProperties.highlightRange = new ProtoCore.CodeModel.CodeRange
{
StartInclusive = new ProtoCore.CodeModel.CodePoint
{
StartInclusive = new ProtoCore.CodeModel.CodePoint
{
LineNo = Constants.kInvalidIndex,
CharNo = Constants.kInvalidIndex
},

EndExclusive = new ProtoCore.CodeModel.CodePoint
{
LineNo = Constants.kInvalidIndex,
CharNo = Constants.kInvalidIndex
}
};

// Save the pc and store it after the call
EmitFunctionCallNode(inlineCall, ref inferedType, false, graphNode, ProtoCore.CompilerDefinitions.SubCompilePass.UnboundIdentifier);
EmitFunctionCallNode(inlineCall, ref inferedType, false, graphNode, ProtoCore.CompilerDefinitions.SubCompilePass.None, parentNode);
LineNo = Constants.kInvalidIndex,
CharNo = Constants.kInvalidIndex
},

// Need to restore those settings.
if (graphNode != null)
EndExclusive = new ProtoCore.CodeModel.CodePoint
{
graphNode.isInlineConditional = isInlineConditionalFlag;
graphNode.updateBlock.startpc = startPC;
graphNode.isReturn = isReturn;
LineNo = Constants.kInvalidIndex,
CharNo = Constants.kInvalidIndex
}
};

// Save the pc and store it after the call
EmitFunctionCallNode(inlineCall, ref inferedType, false, graphNode,
ProtoCore.CompilerDefinitions.SubCompilePass.UnboundIdentifier);
EmitFunctionCallNode(inlineCall, ref inferedType, false, graphNode,
ProtoCore.CompilerDefinitions.SubCompilePass.None, parentNode);

// Need to restore those settings.
if (graphNode != null)
{
graphNode.isInlineConditional = isInlineConditionalFlag;
graphNode.updateBlock.startpc = startPC;
graphNode.isReturn = isReturn;
}
}

Expand Down
2 changes: 1 addition & 1 deletion src/Engine/ProtoCore/DSASM/Executable.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -141,7 +141,7 @@ public class CodeBlock
/// <param name="procTable"></param>
/// <param name="isBreakableBlock"></param>
/// <param name="core"></param>
public CodeBlock(Guid guid, CodeBlockType type, ProtoCore.Language langId, int cbID, SymbolTable symbols, ProcedureTable procTable, bool isBreakableBlock = false, ProtoCore.Core core = null)
public CodeBlock(Guid guid, CodeBlockType type, ProtoCore.Language langId, int cbID, SymbolTable symbols, ProcedureTable procTable, bool isBreakableBlock, ProtoCore.Core core)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is a breaking change.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok, you are correct, I will revert it. However, these default arguments are used in the code without being checked for being null. It looks like the code has been this way for a while, not sure what you would like to happen here (other than no changes).

{
this.guid = guid;
blockType = type;
Expand Down
4 changes: 2 additions & 2 deletions src/Engine/ProtoCore/Parser/AssociativeAST.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1992,16 +1992,16 @@ public class InlineConditionalNode : AssociativeNode
public AssociativeNode ConditionExpression { get; set; }
public AssociativeNode TrueExpression { get; set; }
public AssociativeNode FalseExpression { get; set; }

[Obsolete("IsAutogenerated property is deprecated and is not used. Please remove in 3.0")]
public bool IsAutoGenerated { get; set; }

public InlineConditionalNode()
{
IsAutoGenerated = false;
}

public InlineConditionalNode(InlineConditionalNode rhs) : base(rhs)
{
IsAutoGenerated = false;
ConditionExpression = NodeUtils.Clone(rhs.ConditionExpression);
TrueExpression = NodeUtils.Clone(rhs.TrueExpression);
FalseExpression = NodeUtils.Clone(rhs.FalseExpression);
Expand Down
Loading