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

rustdoc: Remove def_id_no_primitives #92711

Merged
merged 6 commits into from
Jan 30, 2022
Merged

rustdoc: Remove def_id_no_primitives #92711

merged 6 commits into from
Jan 30, 2022

Conversation

zredb
Copy link
Contributor

@zredb zredb commented Jan 10, 2022 •
edited by camelid
Loading

Fixes #90187.

@rustbot rustbot added the T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. label Jan 10, 2022
@rust-highfive
Copy link
Collaborator

Some changes occurred in clean/types.rs.

cc @camelid

@rust-highfive
Copy link
Collaborator

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @CraftSpider (or someone else) soon.

Please see the contribution instructions for more information.

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jan 10, 2022
@jackh726
Copy link
Member

@zredb please try to write more informative PR titles and commit messages. It helps keeping track of changes and such

@rust-log-analyzer

This comment has been minimized.

Sign in to view
@camelid
Copy link
Member

camelid commented Jan 10, 2022

Also, please stop closing your PRs and opening new ones. Just push (or force push if necessary with push -f) to the branch of your old PR.

CraftSpider
CraftSpider reviewed Jan 12, 2022
View reviewed changes
Copy link
Contributor

@CraftSpider CraftSpider left a comment

Choose a reason for hiding this comment

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

Have some quick questions

src/librustdoc/clean/types.rs Show resolved Hide resolved
src/librustdoc/formats/cache.rs Outdated Show resolved Hide resolved
@zredb
Copy link
Contributor Author

zredb commented Jan 13, 2022

@jackh726 @camelid : got it.

@camelid
Copy link
Member

camelid commented Jan 13, 2022

Thanks, I really appreciate it! And please feel free to post here or on Zulip if you run into any issues :)

@camelid
Copy link
Member

camelid commented Jan 13, 2022

It looks like you'll need to update the test src/test/rustdoc-js/primitive.rs.

@camelid
Copy link
Member

camelid commented Jan 13, 2022 •
edited
Loading

Now that you've removed all the uses of def_id_no_primitives, can you remove the definition of the function itself?

@zredb
Copy link
Contributor Author

zredb commented Jan 14, 2022 •
edited
Loading

I'm not sure how to update the test, could you give me any suggestion?

@zredb zredb requested a review from camelid January 14, 2022 12:12
@camelid
Copy link
Member

camelid commented Jan 14, 2022

Actually, it looks like that test is failing because a generic parameter is erroneously being picked up in the search index. cc @GuillaumeGomez

@GuillaumeGomez
Copy link
Member

Looks good to me (except for the nit but doesn't really matter here).

Want to take a last look @camelid?

@GuillaumeGomez
Copy link
Member

Ah nevermind, just saw the error. Checking locally what changed...

zredb and others added 5 commits January 17, 2022 13:41
@zredb @GuillaumeGomez
fix #90187
ff88b59
@zredb @GuillaumeGomez
fix #90187
0dd2703
@zredb @GuillaumeGomez
fix #90187: Replace all def_id_no_primitives with def_id
be9d633
@zredb @GuillaumeGomez
fix #90187
16679a6 
remove the definition of def_id_no_primitives and change;
 a missing use was modified;
 narrow the Cache accessibility of BadImplStripper;
@GuillaumeGomez
Correctly handle pure generics
0a95cef
@GuillaumeGomez
Copy link
Member

@zredb Your PR allowed to discover an already existing bug. I pushed on commit on your branch which should have fixed it.

cc @camelid to review my commit. :)

@camelid camelid assigned camelid and unassigned camelid Jan 17, 2022
@rust-log-analyzer
Copy link
Collaborator

A job failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)

@notriddle
Copy link
Contributor

@bors retry

@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 Jan 30, 2022
@bors
Copy link
Contributor

bors commented Jan 30, 2022

⌛ Testing commit 5cc32e7 with merge 24b05ae2ca8753724d7e1b9eb233f95c0f4710f6...

@bors
Copy link
Contributor

bors commented Jan 30, 2022

💥 Test timed out

@bors bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Jan 30, 2022
@rust-log-analyzer
Copy link
Collaborator

A job failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)

@GuillaumeGomez
Copy link
Member

@bors: retry

@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 Jan 30, 2022
@bors
Copy link
Contributor

bors commented Jan 30, 2022

⌛ Testing commit 5cc32e7 with merge 5e0ddf847245e911ee8c8c39ade001aa364f1bb3...

@bors
Copy link
Contributor

bors commented Jan 30, 2022

💥 Test timed out

@bors bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Jan 30, 2022
@rust-log-analyzer
Copy link
Collaborator

A job failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)

@GuillaumeGomez
Copy link
Member

Weird... Still not linked to this PR though apparently... Let's retry one last time and then I'll ask someone from infra.

@bors: retry

@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 Jan 30, 2022
@bors
Copy link
Contributor

bors commented Jan 30, 2022

⌛ Testing commit 5cc32e7 with merge 08df8b8...

@bors
Copy link
Contributor

bors commented Jan 30, 2022

☀️ Test successful - checks-actions
Approved by: notriddle
Pushing 08df8b8 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Jan 30, 2022
@bors bors merged commit 08df8b8 into rust-lang:master Jan 30, 2022
@rustbot rustbot added this to the 1.60.0 milestone Jan 30, 2022
@bors bors mentioned this pull request Jan 30, 2022
Closed
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (08df8b8): comparison url.

Summary: This benchmark run did not return any relevant results.

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

@rustbot label: -perf-regression

@zredb zredb deleted the issue-90187-fix branch February 4, 2022 13:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@CraftSpider CraftSpider CraftSpider left review comments

@GuillaumeGomez GuillaumeGomez GuillaumeGomez approved these changes

@notriddle notriddle notriddle approved these changes

@camelid camelid Awaiting requested review from camelid

Assignees

@notriddle notriddle

Labels
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. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
1.60.0
Development

Successfully merging this pull request may close these issues.

rustdoc: Replace all remaining uses of Type::def_id_no_primitives() with Type::def_id()