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

Include start offset in Comment #4069

Closed
overlookmotel opened this issue Jul 6, 2024 · 6 comments · Fixed by #4132
Closed

Include start offset in Comment #4069

overlookmotel opened this issue Jul 6, 2024 · 6 comments · Fixed by #4132
Labels
A-ast Area - AST C-cleanup Category - technical debt or refactoring. Solution not expected to change behavior good first issue Experience Level - Good for newcomers

Comments

@overlookmotel
Copy link
Collaborator

#4045 replaced TriviasMap with a sorted Vec (bravo @lucab!)

After that change, there's no need to use a Vec<(u32, Comment)> - can change it to Vec<Comment> and move start field into Comment itself. Or, probably even better, make it struct Comment { span: Span, kind: CommentKind } to match AST nodes.

This would feel more natural to me - start was only external to Comment previously as it was key for the map.

Side note: I'm not sure if SortedComments should be a boxed slice rather than a Vec. Possible that some transforms may want to add/remove comments.

@overlookmotel overlookmotel added C-cleanup Category - technical debt or refactoring. Solution not expected to change behavior A-ast Area - AST labels Jul 6, 2024
@overlookmotel
Copy link
Collaborator Author

@lucab By the way, FYI I made one other small change to your impl - #4067.

@Boshen Boshen added the good first issue Experience Level - Good for newcomers label Jul 6, 2024
@lucab
Copy link
Collaborator

lucab commented Jul 6, 2024

I did start going down the route of a Comment.start refactor in #4045, but then I backtracked and kept a smaller PR. I don't remember exactly, but I think I saw some consumer somewhere that wanted to touch comments positions, and I was a bit scared to break some more distant logic.
A span field makes sense. I'm not very familar with the AST design, but an alternative idea would be to store start and len instead. That would allow to shift comments around just by offsetting their start position.

I'm not sure if SortedComments should be a boxed slice rather than a Vec. Possible that some transforms may want to add/remove comments.

I did not see any library methods for consumers to manipulate trivia.comments, so I think that under the current API they are effectively immutable. If that's going to change, I think there is a need of some further wrappers otherwise consumers can easily break the "sorted" invariant by mistake.

Similar discussion point for irregular_whitespaces: they are currently stored in an immutable Vec.

@Boshen
Copy link
Member

Boshen commented Jul 6, 2024

I think start and end fields with a .span() method should cover existing scenarios.

Trivias are immutable for the foreseeable future so it's fine to keep them as immutable. Or maybe we can encapsulate it for a stable API, to allow us to add mutable methods in the future.

@overlookmotel
Copy link
Collaborator Author

Thanks both for coming back. Yes I see the argument for keeping SortedComments as an immutable boxed slice. As you said @lucab, I guess irregular_whitespaces could/should be a boxed slice then too.

@lucab
Copy link
Collaborator

lucab commented Jul 8, 2024

Ack. If nobody is in a hurry, I can tackle this one later in the week.

@overlookmotel
Copy link
Collaborator Author

No hurry at all!

Boshen pushed a commit that referenced this issue Jul 9, 2024
This tweaks `Comment` definition in order to internally store the start
and end position of its span.

Closes: #4069
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-ast Area - AST C-cleanup Category - technical debt or refactoring. Solution not expected to change behavior good first issue Experience Level - Good for newcomers
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants