Remove header field from clean::Function by GuillaumeGomez · Pull Request #95096 · rust-lang/rust

GuillaumeGomez · 2022-03-19T00:04:31Z

This is another take on #89673 (compared to #91217) but very different on the approach: I moved the header call in one place but still require to have the clean::Item so I can use the DefId to get what is missing.

cc @jyn514 (you reviewed the original so maybe you want to take a look?)
r? @camelid

rust-highfive · 2022-03-19T00:04:34Z

Some changes occurred in clean/types.rs.

cc @camelid

GuillaumeGomez · 2022-03-19T10:06:45Z

Forgot to run the perf check.

@bors try @rust-timer queue

rust-timer · 2022-03-19T10:06:47Z

Awaiting bors try build completion.

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

bors · 2022-03-19T10:06:53Z

⌛ Trying commit 460442576cda0a8e3f6a36023cbfc96d15e1124a with merge adc3dafa0f99c69b5c77712a174931437f4701a3...

bors · 2022-03-19T11:26:37Z

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

rust-timer · 2022-03-19T11:26:38Z

Queued adc3dafa0f99c69b5c77712a174931437f4701a3 with parent 3153584, future comparison URL.

GuillaumeGomez · 2022-03-19T12:30:57Z

Applied suggestions.

rust-timer · 2022-03-19T12:48:18Z

Finished benchmarking commit (adc3dafa0f99c69b5c77712a174931437f4701a3): comparison url.

Summary: This benchmark run shows 39 relevant improvements 🎉 to instruction counts.

Arithmetic mean of relevant improvements: -0.9%
Largest improvement in instruction counts: -1.4% on full builds of issue-46449 doc

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

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR led to changes in compiler perf.

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

GuillaumeGomez · 2022-03-19T16:12:52Z

Nice improvements overall! For the rss usage, we have -0.93% on primary benchmarks and -1.16% on secondary benchmarks.

I also added a test which reexports a foreign function from a dependency to be sure there is no issue with the HirId.

GuillaumeGomez · 2022-03-24T15:19:00Z

Updated!

camelid · 2022-03-27T20:56:50Z

I noticed several places where the constness is overrided like this. I don't quite understand why this code existed. Does the new, on-demand computation account for the old overriding behavior?

Likewise, some places used is_const_fn_raw while others used is_const_fn. Is this handled properly?

It was more or less the same code done by different functions. We have a lot of rustdoc tests for functions and methods to ensure their signature is as expected (I know it because I broke a lot of them while working on this PR).

Ok, seems good to me then. It's good to know the tests worked as intended and failed with previous versions of the code :)

camelid

r=me with the following comments addressed

Thanks for this refactor!

camelid · 2022-03-28T22:56:02Z

Ok, seems good to me then. It's good to know the tests worked as intended and failed with previous versions of the code :)

…handled

…temKind::MethodItem in fn_header function

GuillaumeGomez · 2022-03-29T12:44:56Z

@bors: r=camelid rollup=never

bors · 2022-03-29T12:44:58Z

📌 Commit 8071332 has been approved by camelid

bors · 2022-03-29T12:49:06Z

⌛ Testing commit 8071332 with merge 11909e3...

bors · 2022-03-29T15:18:50Z

☀️ Test successful - checks-actions
Approved by: camelid
Pushing 11909e3 to master...

rust-timer · 2022-03-29T17:04:47Z

Finished benchmarking commit (11909e3): comparison url.

Summary: This benchmark run shows 30 relevant improvements 🎉 to instruction counts.

Arithmetic mean of relevant regressions: 0.3%
Arithmetic mean of relevant improvements: -0.9%
Arithmetic mean of all relevant changes: -0.8%
Largest improvement in instruction counts: -1.4% on full builds of await-call-tree doc

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

@rustbot label: -perf-regression

…g, r=notriddle Remove unneeded `to_string` call Fixes a confusion I made when reading `@camelid's` comment [here](rust-lang#95096 (comment)). r? `@notriddle`

rust-highfive assigned camelid Mar 19, 2022

rustbot added the T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. label Mar 19, 2022

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Mar 19, 2022

jyn514 reviewed Mar 19, 2022
View reviewed changes

Comment threadsrc/librustdoc/clean/types.rs Outdated
Comment threadsrc/librustdoc/clean/types.rs Outdated
Comment threadsrc/librustdoc/clean/types.rs Outdated

rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Mar 19, 2022

GuillaumeGomez force-pushed the rm-header-fn-field branch from 4604425 to e1dc7edCompare March 19, 2022 12:30

rustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Mar 19, 2022

camelid requested changes Mar 23, 2022
View reviewed changes

Comment threadsrc/librustdoc/clean/types.rs Outdated

camelid 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 Mar 23, 2022

GuillaumeGomez force-pushed the rm-header-fn-field branch from 98cefd8 to d135230Compare March 24, 2022 13:36

GuillaumeGomez 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 Mar 24, 2022

GuillaumeGomez force-pushed the rm-header-fn-field branch 2 times, most recently from b0798bd to 8ce764cCompare March 24, 2022 13:47

camelid reviewed Mar 27, 2022
View reviewed changes

camelid 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 Mar 27, 2022

GuillaumeGomez removed the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Mar 28, 2022

GuillaumeGomez added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Mar 28, 2022

camelid approved these changes Mar 28, 2022
View reviewed changes

camelid 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 Mar 28, 2022

GuillaumeGomez added 3 commits March 29, 2022 11:46

Remove header field from clean::Function
2c4ce9d

Add test to ensure foreign functions from dependencies are correctly …
fd48ea0
…handled

Merge ItemKind::TyMethodItem branch with ItemKind::FunctionItem and I…
8071332
…temKind::MethodItem in fn_header function

GuillaumeGomez force-pushed the rm-header-fn-field branch from 8ce764c to 8071332Compare March 29, 2022 10:09

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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Mar 29, 2022

bors added the merged-by-bors This PR was explicitly merged by bors. label Mar 29, 2022

bors merged commit 11909e3 into rust-lang:masterMar 29, 2022

rustbot added this to the 1.61.0 milestone Mar 29, 2022

bors mentioned this pull request Mar 29, 2022
Implement #[deprecated_safe] #95025
Closed

GuillaumeGomez deleted the rm-header-fn-field branch March 29, 2022 15:32

bors mentioned this pull request Mar 29, 2022
Rustdoc: Discriminate required and provided associated constants and types #95316
Merged

GuillaumeGomez mentioned this pull request Mar 30, 2022
Remove unneeded to_string call #95495
Merged

Uh oh!

Uh oh!

Conversation

GuillaumeGomez commented Mar 19, 2022

Uh oh!

rust-highfive commented Mar 19, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

GuillaumeGomez commented Mar 19, 2022

Uh oh!

rust-timer commented Mar 19, 2022

Uh oh!

bors commented Mar 19, 2022

Uh oh!

bors commented Mar 19, 2022

Uh oh!

rust-timer commented Mar 19, 2022

Uh oh!

GuillaumeGomez commented Mar 19, 2022

Uh oh!

rust-timer commented Mar 19, 2022

Uh oh!

GuillaumeGomez commented Mar 19, 2022

Uh oh!

Uh oh!

GuillaumeGomez commented Mar 24, 2022

Uh oh!

camelidMar 27, 2022

Choose a reason for hiding this comment

Uh oh!

camelidMar 27, 2022

Choose a reason for hiding this comment

Uh oh!

GuillaumeGomezMar 28, 2022

Choose a reason for hiding this comment

Uh oh!

camelidMar 28, 2022

Choose a reason for hiding this comment

Uh oh!

camelid left a comment

Choose a reason for hiding this comment

Uh oh!

camelidMar 28, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

GuillaumeGomez commented Mar 29, 2022

Uh oh!

bors commented Mar 29, 2022

Uh oh!

bors commented Mar 29, 2022

Uh oh!

bors commented Mar 29, 2022

Uh oh!

rust-timer commented Mar 29, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants