-
Notifications
You must be signed in to change notification settings - Fork 158
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.
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
feat: add SourceLocation to Node #885
Conversation
Thank you! A few questions/comments about this approach:
This isn't obvious to me. Storing location info in block variants could incur some overhead (i.e., for vector pointers) - but my guess is that this overhead would be under 5% for vast majority of programs. However, for programs where source location is not available, this approach would basically double the overhead (since we'd be just storing default Another things that is not clear to me with this approach is how do we track location of block start/end instructions. For example, in the following program:
Where would locations of Lastly, it seems to me (though, I'm not 100% sure) that the approach described in #866 (comment) would result in lighter code changes. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you! Looks good! I left some comments inline.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you! Looks good! I left a few more small comments.
I think we are pretty close with this one to being done. As the next step, would be good to do a PR which implements serialization/deserialization of source data. I'd probably make that PR on top of this one, and then we'd merge both PRs together.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All looks good! Thank you! I left a couple non-blocking nits inline.
But before we merge this, let's do another PR on top of this one to implement serialization/deserialization.
This commit adds source location to parsed nodes, allowing the construction of source mapping. It modifies the [Node] structure based on the discussion that took place here:
58ded25
to
b2acbc5
Compare
2e66d11
to
3f67236
Compare
This commit adds source location to parsed nodes, allowing the construction of source mapping.
It modifies the [Node] structure based on the discussion that took place here:
#866 (comment)
The implementation is slightly different than the one discussed as every instruction individually will contain a [SourceLocation] - it will be less expensive to store it like that instead of creating sets for each of the block variants.
The [SourceLocation] itself is a cheap type that will contain integers only.
This commit doesn't touch the serialization question for ModuleAst with SourceLocation. These additional bytes might be optional, and including them might be optional as they will consume additional production storage when they will provide functionality only for debug/development environments.