fix: debug info print with depth#5903
Merged
kripken merged 1 commit intoWebAssembly:mainfrom Aug 28, 2023
Merged
Conversation
Contributor
Author
|
@kripken |
kripken
reviewed
Aug 28, 2023
Member
kripken
left a comment
kripken
left a comment
There was a problem hiding this comment.
Thanks! Looks good with some comments, see the suggestion. Is what I wrote there correct?
|
|
||
| std::vector<HeapType> heapTypes; | ||
|
|
||
| unsigned lastPrintIndent = 0; |
Member
There was a problem hiding this comment.
Please add a comment explaining the motivation for tracking the print indent, maybe with an example? I am imagining something like this:
// Track the print indent so that we can see when it changes. That affects how we print debug annotations. In particular, we don't want to print repeated debug locations for children, like this:
//
// ;;@ file.cpp:20:4
// (block
// ;; no need to annotate here
// (nop)
//
// But we do want to print an annotation even if it repeats if it is not a child:
//
// ;;@ file.cpp:20:4
// (block
// )
// ;;@ file.cpp:20:4 - this is worth annotating
// (nop)
kripken
approved these changes
Aug 28, 2023
Member
kripken
left a comment
kripken
left a comment
There was a problem hiding this comment.
Actually let me land this - I have a followup to this code, and there would just be merge conflicts etc. I'll add a comment in my PR.
kripken
added a commit
that referenced
this pull request
Aug 29, 2023
In general, full print mode should print out all the things to avoid confusion. It already did so for blocks (that the text format sometimes elides), types, etc. Doing it for debug info can avoid confusion when debugging (in fact, this was one of the main reasons I've been confused about how source maps work in Binaryen...). Also add a comment to the code just landed in #5903
radekdoulik
pushed a commit
to dotnet/binaryen
that referenced
this pull request
Jul 12, 2024
Skip repeated identical debug info only of more-nested nodes. Before this PR we skipped sibling nodes and even parent nodes, which could be confusing. After this PR there is a more clear connection: child nodes have the same debug location as the parent, by default, and so there is no need to print it again.
radekdoulik
pushed a commit
to dotnet/binaryen
that referenced
this pull request
Jul 12, 2024
In general, full print mode should print out all the things to avoid confusion. It already did so for blocks (that the text format sometimes elides), types, etc. Doing it for debug info can avoid confusion when debugging (in fact, this was one of the main reasons I've been confused about how source maps work in Binaryen...). Also add a comment to the code just landed in WebAssembly#5903
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Split from #5547.
Focus on debug info print optimization.