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.

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

Merged
merged 2 commits into from
May 19, 2023
Merged

Conversation

vlopes11
Copy link
Contributor

@vlopes11 vlopes11 commented May 2, 2023

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.

@vlopes11 vlopes11 requested review from bobbinth and grjte May 2, 2023 00:46
@bobbinth
Copy link
Contributor

bobbinth commented May 2, 2023

Thank you! A few questions/comments about this approach:

it will be less expensive to store it like that instead of creating sets for each of the block variants.

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 SourceLocation structs in each node, instead of empty vectors in each block).

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:

while.true
...
end

Where would locations of while.true and end instructions be tracked?

Lastly, it seems to me (though, I'm not 100% sure) that the approach described in #866 (comment) would result in lighter code changes.

Copy link
Contributor

@bobbinth bobbinth left a 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.

assembly/src/parsers/nodes.rs Outdated Show resolved Hide resolved
assembly/src/parsers/mod.rs Outdated Show resolved Hide resolved
assembly/src/parsers/mod.rs Outdated Show resolved Hide resolved
assembly/src/parsers/mod.rs Outdated Show resolved Hide resolved
assembly/src/tokens/stream.rs Outdated Show resolved Hide resolved
assembly/src/parsers/context.rs Outdated Show resolved Hide resolved
@vlopes11 vlopes11 requested a review from bobbinth May 11, 2023 01:20
Copy link
Contributor

@bobbinth bobbinth left a 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.

assembly/src/parsers/nodes.rs Outdated Show resolved Hide resolved
assembly/src/parsers/serde/deserialization.rs Outdated Show resolved Hide resolved
assembly/src/parsers/context.rs Outdated Show resolved Hide resolved
assembly/src/parsers/context.rs Show resolved Hide resolved
assembly/src/parsers/context.rs Outdated Show resolved Hide resolved
assembly/src/parsers/context.rs Outdated Show resolved Hide resolved
assembly/src/parsers/mod.rs Outdated Show resolved Hide resolved
@vlopes11 vlopes11 requested a review from bobbinth May 15, 2023 12:59
Copy link
Contributor

@bobbinth bobbinth left a 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.

assembly/src/parsers/mod.rs Outdated Show resolved Hide resolved
assembly/src/parsers/mod.rs Outdated Show resolved Hide resolved
@grjte grjte removed their request for review May 18, 2023 12:34
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:
@vlopes11 vlopes11 force-pushed the vlopes11-node-source-location branch from 58ded25 to b2acbc5 Compare May 19, 2023 16:37
@vlopes11 vlopes11 force-pushed the vlopes11-node-source-location branch from 2e66d11 to 3f67236 Compare May 19, 2023 17:06
@vlopes11 vlopes11 merged commit 91654e1 into next May 19, 2023
@vlopes11 vlopes11 deleted the vlopes11-node-source-location branch May 19, 2023 17:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants