Separate out a `hir::Impl` struct #79322

jyn514 · 2020-11-22T22:51:52Z

This makes it possible to pass the Impl directly to functions, instead
of having to pass each of the many fields one at a time. It also
simplifies matches in many cases.

See rustc_save_analysis::dump_visitor::process_impl or rustdoc::clean::clean_impl for a good example of how this makes impls easier to work with.

r? @petrochenkov maybe?

jyn514 · 2020-11-22T22:59:50Z

Another thing I noticed is that it's really common to match on Impl just to see whether it's a trait or an inherent impl - maybe it makes sense to add Impl::is_inherent() so this is easier to write? I can add that either here or in a follow-up.

petrochenkov · 2020-11-23T20:12:18Z

This would be an excellent use case for rust-lang/rfcs#2593 ("Enum variant types").

petrochenkov · 2020-11-23T20:13:26Z

I don't know.
This change improves code in one place in dump_visitor, but adds noise in numerous other places.

jyn514 · 2020-11-24T00:19:48Z

If it helps, it would also improve some rustdoc code, that was the original use case.

petrochenkov · 2020-11-24T17:34:47Z

Need second opinion.
r? @estebank as GitHub suggests.

estebank

I think that Impl had grown a lot of fields so this makes a lot of sense. I would take more opportunities to use the nested types destructuring to avoid using field access, but this is fine as is.

r=me.

estebank · 2020-11-24T20:40:23Z

src/librustdoc/visit_ast.rs

+            hir::ItemKind::Impl(ref impl_) => {
                // Don't duplicate impls when inlining or if it's implementing a trait, we'll pick
                // them up regardless of where they're located.
-                if !self.inlining && of_trait.is_none() {
-                    let items =
-                        items.iter().map(|item| self.cx.tcx.hir().impl_item(item.id)).collect();
+                if !self.inlining && impl_.of_trait.is_none() {
+                    let items = impl_
+                        .items
+                        .iter()
+                        .map(|item| self.cx.tcx.hir().impl_item(item.id))
+                        .collect();
                    let i = Impl {
-                        unsafety,
-                        polarity,
-                        defaultness,
-                        constness,
-                        generics,
-                        trait_: of_trait,
-                        for_: self_ty,
+                        unsafety: impl_.unsafety,
+                        polarity: impl_.polarity,
+                        defaultness: impl_.defaultness,
+                        constness: impl_.constness,
+                        generics: &impl_.generics,
+                        trait_: &impl_.of_trait,
+                        for_: &impl_.self_ty,


I think that this bit would be slightly nicer by using destructuring as it was before by using hir::ItemKind::Impl(hir::Impl { .. }) => like you do in other places.

This whole block of code disappeared when rebasing over #79312; the new changes are in https://github.com/rust-lang/rust/pull/79322/files#diff-384affc1b4190940f114f3fcebbf969e7e18657a71ef9001da6b223a036687d9R2037. I think you'll agree the new code is a lot nicer :)

jyn514 · 2020-12-16T02:45:31Z

I fixed the merge conflicts. @estebank could you take another look? This bitrots rather quickly.

bors · 2020-12-21T07:17:57Z

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

Note that reviewers usually do not review pull requests until merge conflicts are resolved! Once you resolve the conflicts, you should change the labels applied by bors to indicate that your PR is ready for review. Post this as a comment to change the labels:

@rustbot modify labels: +S-waiting-on-review -S-waiting-on-author

estebank

@jyn514 sorry about the delay. Please rebase and r=me.

estebank · 2021-01-12T18:26:23Z

compiler/rustc_hir/src/hir.rs

-        self_ty: &'hir Ty<'hir>,
-        items: &'hir [ImplItemRef<'hir>],
-    },
+    Impl(Impl<'hir>),


I wonder if Boxing this would have any perf benefits 👀

I opened #80961 to find out :)

This makes it possible to pass the `Impl` directly to functions, instead of having to pass each of the many fields one at a time. It also simplifies matches in many cases.

jyn514 · 2021-01-13T01:34:35Z

@bors r=estebank rollup=iffy

bors · 2021-01-13T01:34:36Z

📌 Commit a8ff647 has been approved by estebank

bors · 2021-01-13T01:40:47Z

⌛ Testing commit a8ff647 with merge 150d1fe...

bors · 2021-01-13T04:29:39Z

☀️ Test successful - checks-actions
Approved by: estebank
Pushing 150d1fe to master...

jyn514 added C-cleanup Category: PRs that clean code up or issues documenting cleanup. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Nov 22, 2020

This comment has been minimized.

Sign in to view

rust-highfive assigned petrochenkov Nov 22, 2020

This comment has been minimized.

Sign in to view

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Nov 22, 2020

jyn514 force-pushed the refactor-impl branch 2 times, most recently from a88a906 to c417d8d Compare November 22, 2020 22:53

This comment has been minimized.

Sign in to view

jyn514 added the I-needs-decision Issue: In need of a decision. label Nov 24, 2020

rust-highfive assigned estebank and unassigned petrochenkov Nov 24, 2020

estebank approved these changes Nov 24, 2020

View reviewed changes

jyn514 force-pushed the refactor-impl branch from c417d8d to 925a48d Compare November 27, 2020 02:16

jyn514 force-pushed the refactor-impl branch 2 times, most recently from 55083ce to 485a980 Compare December 16, 2020 02:45

camelid added the A-HIR Area: The high-level intermediate representation (HIR) label Dec 22, 2020

estebank approved these changes Jan 12, 2021

View reviewed changes

Separate out a hir::Impl struct

a8ff647

This makes it possible to pass the `Impl` directly to functions, instead of having to pass each of the many fields one at a time. It also simplifies matches in many cases.

jyn514 force-pushed the refactor-impl branch from 485a980 to a8ff647 Compare January 13, 2021 01:32

bors removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jan 13, 2021

bors added the S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. label Jan 13, 2021

jyn514 mentioned this pull request Jan 13, 2021

Box ItemKind::Impl #80961

Closed

bors added the merged-by-bors This PR was explicitly merged by bors. label Jan 13, 2021

bors merged commit 150d1fe into rust-lang:master Jan 13, 2021

rustbot added this to the 1.51.0 milestone Jan 13, 2021

jyn514 deleted the refactor-impl branch January 13, 2021 04:38

This was referenced Jan 13, 2021

Store HIR attributes in a side table #79519

Merged

Use Option::map_or instead of .map(..).unwrap_or(..) #80944

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Separate out a `hir::Impl` struct #79322

Separate out a `hir::Impl` struct #79322

jyn514 commented Nov 22, 2020 •

edited

Loading

This comment has been minimized.

This comment has been minimized.

jyn514 commented Nov 22, 2020

petrochenkov commented Nov 23, 2020

petrochenkov commented Nov 23, 2020

jyn514 commented Nov 24, 2020

This comment has been minimized.

petrochenkov commented Nov 24, 2020

estebank left a comment

estebank Nov 24, 2020

jyn514 Nov 27, 2020

jyn514 commented Dec 16, 2020

bors commented Dec 21, 2020

estebank left a comment

estebank Jan 12, 2021

jyn514 Jan 13, 2021

jyn514 commented Jan 13, 2021

bors commented Jan 13, 2021

bors commented Jan 13, 2021

bors commented Jan 13, 2021

Separate out a hir::Impl struct #79322

Separate out a hir::Impl struct #79322

Conversation

jyn514 commented Nov 22, 2020 • edited Loading

This comment has been minimized.

This comment has been minimized.

jyn514 commented Nov 22, 2020

petrochenkov commented Nov 23, 2020

petrochenkov commented Nov 23, 2020

jyn514 commented Nov 24, 2020

This comment has been minimized.

petrochenkov commented Nov 24, 2020

estebank left a comment

Choose a reason for hiding this comment

estebank Nov 24, 2020

Choose a reason for hiding this comment

jyn514 Nov 27, 2020

Choose a reason for hiding this comment

jyn514 commented Dec 16, 2020

bors commented Dec 21, 2020

estebank left a comment

Choose a reason for hiding this comment

estebank Jan 12, 2021

Choose a reason for hiding this comment

jyn514 Jan 13, 2021

Choose a reason for hiding this comment

jyn514 commented Jan 13, 2021

bors commented Jan 13, 2021

bors commented Jan 13, 2021

bors commented Jan 13, 2021

Separate out a `hir::Impl` struct #79322

Separate out a `hir::Impl` struct #79322

jyn514 commented Nov 22, 2020 •

edited

Loading