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

Remove some function fields #80883

Merged
merged 6 commits into from
Feb 6, 2021
Merged

Remove some function fields #80883

merged 6 commits into from
Feb 6, 2021

Conversation

GuillaumeGomez
Copy link
Member

@GuillaumeGomez GuillaumeGomez commented Jan 10, 2021 •
edited
Loading

Same kind as #80845.

This PR removes the all_types and ret_types from the clean::Function type.

Another change that I had to do was implementing the From trait to be able to convert hir::def::DefKind into clean::TypeKind without requiring DocContext (and so I updated the clean method so that it's taken into account).

The last two commits improve a bit the get_real_types function and the Type::generics method.

r? @jyn514

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

Let's check the perf results too while we're at it.

@bors try @rust-timer queue

@rust-timer
Copy link
Collaborator

Awaiting bors try build completion.

@bors
Copy link
Contributor

bors commented Jan 10, 2021

⌛ Trying commit 38100f17d2ad1a3c9f177abf49b0f1e50eb61c1e with merge 1977d4945b2c970bc1d8b2849d0eeb0d0cbb8e24...

@bors
Copy link
Contributor

bors commented Jan 10, 2021

☀️ Try build successful - checks-actions
Build commit: 1977d4945b2c970bc1d8b2849d0eeb0d0cbb8e24 (1977d4945b2c970bc1d8b2849d0eeb0d0cbb8e24)

@rust-timer
Copy link
Collaborator

Queued 1977d4945b2c970bc1d8b2849d0eeb0d0cbb8e24 with parent 080ee6f, future comparison URL.

@rustbot label: +S-waiting-on-perf

@rustbot rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Jan 10, 2021
@rust-timer
Copy link
Collaborator

Finished benchmarking try commit (1977d4945b2c970bc1d8b2849d0eeb0d0cbb8e24): comparison url.

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. Please note that if the perf results are neutral, you should likely undo the rollup=never given below by specifying rollup- to bors.

Importantly, though, if the results of this run are non-neutral do not roll this PR up -- it will mask other regressions or improvements in the roll up.

@bors rollup=never
@rustbot label: +S-waiting-on-review -S-waiting-on-perf

@rustbot rustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Jan 11, 2021
@GuillaumeGomez
Copy link
Member Author

Interesting:

instructions: between -1% and -2.7%
memory: between -0.7% and -4.5%

Much better than what I expected.

@GuillaumeGomez GuillaumeGomez mentioned this pull request Jan 11, 2021
Merged
@jyn514
Copy link
Member

jyn514 commented Jan 11, 2021

@camelid would you like to review the change?

@jyn514 jyn514 added C-cleanup Category: PRs that clean code up or issues documenting cleanup. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. labels Jan 11, 2021
camelid
camelid reviewed Jan 11, 2021
View reviewed changes
Copy link
Member

@camelid camelid left a comment

Choose a reason for hiding this comment

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

Here's a few comments.

src/librustdoc/formats/cache.rs Outdated Show resolved Hide resolved
src/librustdoc/formats/renderer.rs Outdated Show resolved Hide resolved
src/librustdoc/clean/types.rs Outdated Show resolved Hide resolved
@camelid
Copy link
Member

camelid commented Jan 11, 2021

@camelid would you like to review the change?

I don't think I have enough knowledge of this code to be the reviewer, but I did leave a few comments.

@GuillaumeGomez GuillaumeGomez force-pushed the remove-some-function-fields branch from 38100f1 to e4d599e Compare January 11, 2021 21:33
jyn514
jyn514 reviewed Jan 12, 2021
View reviewed changes
src/librustdoc/clean/mod.rs Show resolved Hide resolved
src/librustdoc/clean/types.rs Outdated Show resolved Hide resolved
src/librustdoc/clean/types.rs Show resolved Hide resolved
src/librustdoc/clean/types.rs Outdated Show resolved Hide resolved
src/librustdoc/formats/cache.rs Outdated Show resolved Hide resolved
src/librustdoc/formats/cache.rs Outdated Show resolved Hide resolved
src/librustdoc/html/render/cache.rs Outdated Show resolved Hide resolved
src/librustdoc/formats/cache.rs Outdated Show resolved Hide resolved
@GuillaumeGomez GuillaumeGomez force-pushed the remove-some-function-fields branch 2 times, most recently from c006ffb to b8e5ee6 Compare January 12, 2021 13:09
@jyn514 jyn514 mentioned this pull request Jan 14, 2021
Merged
@bors
Copy link
Contributor

bors commented Jan 21, 2021

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

@camelid camelid added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Jan 28, 2021
@GuillaumeGomez GuillaumeGomez force-pushed the remove-some-function-fields branch from b8e5ee6 to 4c2c0b1 Compare January 28, 2021 15:24
@GuillaumeGomez
Copy link
Member Author

I made the PR back from the start based on the lastest developments. No more need for TyCtxt in Cache in the end, so that's quite positive. :)

Confirming that the performance improvement is still there:

@bors try @rust-timer queue

@rust-timer
Copy link
Collaborator

Awaiting bors try build completion.

@bors
Copy link
Contributor

bors commented Jan 28, 2021

⌛ Trying commit 4c2c0b152785a5826fd45430d7f6e3f087271cd9 with merge bdc8ba6d71c33ac2e37bb4e7da6544dde3d300d3...

@GuillaumeGomez GuillaumeGomez force-pushed the remove-some-function-fields branch from 01d333d to c92b161 Compare February 5, 2021 16:52
@GuillaumeGomez
Copy link
Member Author

Rebased.

@ollie27
Copy link
Member

ollie27 commented Feb 6, 2021

There is some good cleanup here but as I've tried to say the get_index_search_type doesn't need the cache or tcx arguments. I guess they can be removed in a follow up though.

@bors r+

@bors
Copy link
Contributor

bors commented Feb 6, 2021

📌 Commit c92b161 has been approved by ollie27

@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 Feb 6, 2021
@bors
Copy link
Contributor

bors commented Feb 6, 2021

⌛ Testing commit c92b161 with merge 6dd633a726a1900af8a4c6a769809093b6cf3aa1...

@bors
Copy link
Contributor

bors commented Feb 6, 2021

💥 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 Feb 6, 2021
@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 Author

@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 Feb 6, 2021
@bors
Copy link
Contributor

bors commented Feb 6, 2021

⌛ Testing commit c92b161 with merge a73c2e5...

@bors
Copy link
Contributor

bors commented Feb 6, 2021

☀️ Test successful - checks-actions
Approved by: ollie27
Pushing a73c2e5 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Feb 6, 2021
@bors bors merged commit a73c2e5 into rust-lang:master Feb 6, 2021
@rustbot rustbot added this to the 1.52.0 milestone Feb 6, 2021
@GuillaumeGomez GuillaumeGomez deleted the remove-some-function-fields branch February 6, 2021 23:47
@bors bors mentioned this pull request Feb 6, 2021
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Feb 24, 2021
@bors
Auto merge of rust-lang#80914 - GuillaumeGomez:remove-is_spotlight, r…
a8486b6 
…=jyn514

Remove is_spotlight field from `Trait`

Small PR, only the last commit is relevant here. The rest is coming from rust-lang#80883 because I need the `TyCtxt` stored inside `Cache`.

The point is to make ItemKind looks as close as possible to the compiler type so that it makes the switch simpler (which is why I make all these "small" PRs).

r? `@jyn514`
camelid added a commit to camelid/rust that referenced this pull request Dec 28, 2021
@camelid
rustdoc: Remove some unnecessary cache parameters
e19593f 
Based on
rust-lang#80883 (comment).
The `tcx` parameters do seem to be used though, so I only removed the
`cache` parameters.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@camelid camelid camelid left review comments

@jyn514 jyn514 jyn514 left review comments

@calebcartwright calebcartwright calebcartwright left review comments

@ollie27 ollie27 ollie27 left review comments

@bugadani bugadani bugadani left review comments

Assignees

@ollie27 ollie27

@camelid camelid

Labels
C-cleanup Category: PRs that clean code up or issues documenting cleanup. 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.52.0
Development

Successfully merging this pull request may close these issues.