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

Jump to bottom

Handle runtime table gaps on code block deletion #10605

Merged
merged 8 commits into from
May 4, 2020
Merged

Handle runtime table gaps on code block deletion #10605

Changes from 1 commit
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
baed834
Handle runtime table gaps on code block deletion
mmisol Apr 27, 2020
85f7232
Remove commented out code
mmisol Apr 27, 2020
c3bd236
Remove commented out code
mmisol Apr 27, 2020
87b0284
Fix typo and improve comment
mmisol Apr 27, 2020
7a45ab7
Add test
mmisol Apr 27, 2020
f6343bd
Last code block may have children
mmisol Apr 28, 2020
e331114
Fix code block id generation on create
mmisol Apr 28, 2020
84b701b
Add comment for empty list case
mmisol Apr 28, 2020
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
8 changes: 7 additions & 1 deletion src/Engine/ProtoCore/Core.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -770,7 +770,13 @@ public void GenerateExecutable()
private int GetRuntimeTableSize()
QilongTang marked this conversation as resolved.
Show resolved Hide resolved
{
// Due to the way this list is constructed, the largest id is the one of the last block.
return CompleteCodeBlockList.Last().codeBlockId + 1;
var lastBlock = CompleteCodeBlockList.Last();
// If the last block has children, then its last child has the largest id.
if (lastBlock.children.Count > 0)
{
lastBlock = lastBlock.children.Last();
Copy link
Contributor

Choose a reason for hiding this comment

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

I guess I had similar question here why we are checking children of last block but other than this, everything else looks fine to me..

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I logged a ticket for the undo/redo bug. Maybe we can revisit this after we have fixed that? I feel they may be related.

I could also place a comment here mentioning that.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks, would be good if you do that

}
return lastBlock.codeBlockId + 1;
Copy link
Contributor

Choose a reason for hiding this comment

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

If the assertion that the last codeblock always has the largest id is true, then this function needs to be recursive.

Copy link
Collaborator Author

@mmisol mmisol Apr 28, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

I have wondered about that too, but if you take a look at the part of the code I mention here #10605 (comment), you'll see that the code doesn't do recursion, it also looks only at the first level of children. Maybe these code blocks can contain only one level of children in practice?

Copy link
Contributor

@aparajit-pratap aparajit-pratap Apr 28, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

The BfsBuildSequenceTable method is recursive.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@aparajit-pratap Yes, but it's using CodeBlockList which is actually a tree. Both this method and the one I pointed to in my previous comment use CompleteCodeBlockList which contains the same nodes but is flattened, to one level of children it would seem.

Copy link
Contributor

@aparajit-pratap aparajit-pratap Apr 28, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

There should be a codeblock for each new block (language block such as [Imperative {...}], if-else, while, for blocks etc., and function definition) so there can be any number of nesting or children of child codeblocks and so on.

Copy link
Contributor

Choose a reason for hiding this comment

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

Then how can it be that the children can have the largest id? The largest should always be at the top level, i.e. CompleteCodeBlockList?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I don't know if I understood what you meant. What I was saying is that the List.Add method adds to the end of the list, and since we always get a bigger id each time, by construction the biggest id should be at the end of the flattened list. Does that make sense?

Copy link
Contributor

@aparajit-pratap aparajit-pratap Apr 29, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Yes, I understood that. If we're adding to the end of the CompleteCodeBlockList, we simply need to check the size of that list or the codeBlockId of the last item in CompleteCodeBlockList. Why are you checking for children of the last item in the CompleteCodeBlockList?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

we simply need to check the size of that list or the codeBlockId of the last item in CompleteCodeBlockList

If we go back to the purpose of this PR, those two do not always match, so it's not the same to go with one or the other. Just wanted to make that clear.

Why are you checking for children of the last item in the CompleteCodeBlockList?

By way of undo/redo in Josh's graph I was able to see a crash where the last code block contained children with larger ids than the id of its parent or the amount of elements in the list. It does seem strange that the children themselves were not part of the list, so it could be a byproduct of the undo/redo bug.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not so sure I understand the changes, to be honest. If you feel confident you are welcome to get this approved by someone else on the team. I'll do some investigation on my own to try to convince myself of your changes :)

}

public string GenerateTempVar()
Expand Down