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

Move doc comment parsing to rustc_lexer #75642

Merged
merged 2 commits into from
Aug 21, 2020
Merged

Move doc comment parsing to rustc_lexer #75642

merged 2 commits into from
Aug 21, 2020

Conversation

matklad
Copy link
Member

@matklad matklad commented Aug 17, 2020 •
edited
Loading

Plain comments are trivia, while doc comments are not, so it feels
like this belongs to the rustc_lexer.

The specific reason to do this is the desire to use rustc_lexer in
rustdoc for syntax highlighting, without duplicating "is this a doc
comment?" logic there.

r? @ghost

@matklad
Copy link
Member Author

matklad commented Aug 17, 2020

r? @petrochenkov

@petrochenkov petrochenkov added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Aug 17, 2020
petrochenkov
petrochenkov reviewed Aug 18, 2020
View reviewed changes
@petrochenkov
Copy link
Contributor

r=me with nits addressed.

@petrochenkov petrochenkov added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Aug 18, 2020
@matklad
Copy link
Member Author

matklad commented Aug 19, 2020 •
edited
Loading

I've addressed nits except for qualified paths. I think, in this case, qualified paths would be better.

We have parallel hierarchies of types here:

rustc_lexer: Token TokenKind DocCommentKind CommentKind -----------
rustc_ast:   Token TokenKind AttrStyle      ----------- CommentKind

So, it seems better if we stick to not qualifying _ast and qualifing _lexer

@matklad matklad added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Aug 19, 2020
@petrochenkov
Copy link
Contributor 

CommentKind -----------
----------- CommentKind

This looks pretty bad.
I think we need to come up with a different name for one of these.

@petrochenkov
Copy link
Contributor

Block vs Line looks like a "heavier" candidate for CommentKind to me.

The other can be CommentIsDoc::(Yes,No).

Also DocCommentKind -> DocCommentStyle (and change its variant order) to mirror AttrStyle closer.
I'd probably just put AttrStyle as is into the lexer, personally, but there are issues with (en,de)coding and hash impls.

@petrochenkov petrochenkov added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Aug 19, 2020
matklad added 2 commits August 19, 2020 22:53
@matklad
Move doc comment parsing to rustc_lexer
39197e6 
Plain comments are trivial, while doc comments are not, so it feels
like this belongs to the rustc_lexer.

The specific reason to do this is the desire to use rustc_lexer in
rustdoc for syntax highlighting, without duplicating "is this a doc
comment?" logic there.
@matklad
Simplify search for bare \r in doc comments
ccbe94b 
Outer `if` is the fast path -- it calls into hyperoptimized memchr.

The inner loop is just the simplest code possible -- it doesn't
generated the tightest code, but that shouldn't matter if we are going
to error anyhow.
@matklad
Copy link
Member Author

matklad commented Aug 19, 2020

How this version look? I've get rid of CommentKind and renamed DocCommentKind -> DocStyle

@matklad matklad added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Aug 19, 2020
@petrochenkov
Copy link
Contributor

LGTM.
@bors r+

@bors
Copy link
Contributor

bors commented Aug 19, 2020

📌 Commit ccbe94b has been approved by petrochenkov

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Aug 19, 2020
@bors
Copy link
Contributor

bors commented Aug 21, 2020

⌛ Testing commit ccbe94b with merge b51651a...

@bors
Copy link
Contributor

bors commented Aug 21, 2020

☀️ Test successful - checks-actions, checks-azure
Approved by: petrochenkov
Pushing b51651a to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Aug 21, 2020
@bors bors merged commit b51651a into rust-lang:master Aug 21, 2020
@matklad matklad deleted the lexer-comments branch August 21, 2020 08:01
@bors bors mentioned this pull request Aug 21, 2020
Closed
@camelid camelid added the A-parser Area: The parsing of Rust source code to an AST label Oct 19, 2020
@cuviper cuviper added this to the 1.47.0 milestone May 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Mark-Simulacrum Mark-Simulacrum Mark-Simulacrum left review comments

@petrochenkov petrochenkov petrochenkov left review comments

Assignees

@petrochenkov petrochenkov

Labels
A-parser Area: The parsing of Rust source code to an AST merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Milestone
1.47.0
Development

Successfully merging this pull request may close these issues.
6 participants
@matklad @petrochenkov @bors @Mark-Simulacrum @cuviper @camelid