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

Refactor doc comment parsing #74209

Closed
wants to merge 4 commits into from
Closed

Refactor doc comment parsing #74209

wants to merge 4 commits into from

Conversation

tesuji
Copy link
Contributor

@tesuji tesuji commented Jul 10, 2020 •
edited
Loading

Split from #74094 and improved.
I have another way to implement is_doc_comment but might be slower: #74183

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jul 10, 2020
@tesuji
Copy link
Contributor Author

tesuji commented Jul 10, 2020

May need a crater run for rustdoc.

@tesuji
Copy link
Contributor Author

tesuji commented Jul 10, 2020

Actually, this PR needs compiler members review: r? @petrochenkov

@tesuji tesuji changed the title Refactor comment parsing Refactor doc comment parsing Jul 10, 2020
@petrochenkov
Copy link
Contributor

Oh, this looks like something from my todo list.
rustc should never do things like "decoration stripping" and should preserve tokens precisely, making doc comments look presentable to user is purely a rustdoc's job.

@tesuji
Copy link
Contributor Author

tesuji commented Jul 10, 2020

That seems right thing to do. How would I proceed this PR?

@petrochenkov
Copy link
Contributor

(I didn't look at the PR in detail yet, will review later.)

@tesuji tesuji force-pushed the refactor-comment-parsing branch 3 times, most recently from 19cfcb7 to b64a104 Compare July 10, 2020 06:06
tesuji
tesuji commented Jul 10, 2020
View reviewed changes
src/librustc_ast/util/comments.rs Outdated
Comment on lines 69 to 76
// first line of all-stars should be omitted
if !lines.is_empty() && lines[0].chars().all(|c| c == '*') {
i += 1;
}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This condition may seem useless as we never consider /****** [...] */ as doc comment.
I am not sure about /*!******** [..] */, should we keep this behavior.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it was supposed to address the /****** case.
If it's not relevant now, then it's better to simplify the code and remove it.

@tesuji tesuji force-pushed the refactor-comment-parsing branch 2 times, most recently from 36b83df to 7ff5e2a Compare July 11, 2020 02:03
@bors

This comment has been minimized.

Sign in to view
@petrochenkov
Copy link
Contributor

rustc should never do things like "decoration stripping" and should preserve tokens precisely, making doc comments look presentable to user is purely a rustdoc's job.

I'll submit a PR removing the doc comment beautification from the compiler tomorrow.

@petrochenkov
Copy link
Contributor

Otherwise:

  • comments, tests - good
  • introducing a new tiny module for block comments - not good
  • introducing unsafe - not good
  • other changes - not sure, looks like a complete rewrite, but its purpose is not clear to me, will look after rebase on my PR

@petrochenkov
Copy link
Contributor

Blocked on #74627.

@petrochenkov petrochenkov added S-blocked Status: Marked as blocked ❌ on something else such as an RFC or other implementation work. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jul 22, 2020
@petrochenkov
Copy link
Contributor

#74627 has landed.

@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-blocked Status: Marked as blocked ❌ on something else such as an RFC or other implementation work. labels Aug 7, 2020
@tesuji tesuji force-pushed the refactor-comment-parsing branch 2 times, most recently from 0bf2c68 to 2ebd42e Compare August 12, 2020 15:19
@petrochenkov
Copy link
Contributor

I still feel skeptical about many changes here (#74209 (comment)).
Could you keep the tests and comments (and maybe simplifications to fn beautify_doc_string), but avoid other changes?

@tesuji
Copy link
Contributor Author

tesuji commented Aug 13, 2020

Some explanation:

introducing a new tiny module for block comments - not good

vertical_trim and horizontal_trim only used by beautify_doc_string.
I find it messier/bloat to move them under src/librustc_ast/util/comments.rs,
move tests to src/librustc_ast/util/comments/tests.rs.
If you find that's good I will definitely move it back.

other changes - not sure, looks like a complete rewrite, but its purpose
is not clear to me, will look after rebase on my PR

I find the logic straight-forward after rewriting line_doc_comment_style and block_doc_comment_style.

I admit the clippy changes are out of nowhere. Will move it to another PR.

introducing unsafe - not good

Yeah, I admit unsafe is not good. My goal was panic safe.
I tried to document safety invariants and wrote some tests for it.
But honestly I am no expert at writing unsafe.
I will try to replace them with safe functions (but it's hard to find ones panic-free)
if you find them annoying/unnecessary.

@bors
Copy link
Contributor

bors commented Aug 21, 2020

☔ The latest upstream changes (presumably #75642) made this pull request unmergeable. Please resolve the merge conflicts.

@crlf0710
Copy link
Member

@lzutao Ping from triage! CI is still red here. Any updates?

@tesuji
Copy link
Contributor Author

tesuji commented Sep 25, 2020

Sorry, I lost interested in this change. Everyone else could pick it if they want.

@tesuji tesuji closed this Sep 25, 2020
@tesuji tesuji deleted the refactor-comment-parsing branch September 25, 2020 13:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@petrochenkov petrochenkov petrochenkov left review comments

@GuillaumeGomez GuillaumeGomez GuillaumeGomez approved these changes

Assignees

@petrochenkov petrochenkov

Labels
S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@tesuji @petrochenkov @bors @crlf0710 @GuillaumeGomez @Mark-Simulacrum @rust-highfive