-
Notifications
You must be signed in to change notification settings - Fork 13.2k
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.
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
gather body owners #98203
gather body owners #98203
Conversation
r? @nagisa (rust-highfive has picked a reviewer for you, use r? to override) |
r? @cjgillot |
This comment has been minimized.
This comment has been minimized.
@@ -1337,6 +1368,10 @@ pub(crate) fn hir_crate_items(tcx: TyCtxt<'_>, _: ()) -> ModuleItems { | |||
} | |||
|
|||
fn visit_item(&mut self, item: &'hir Item<'hir>) { | |||
if self.tcx.hir().is_body_owner(item.def_id) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about this?
if self.tcx.hir().is_body_owner(item.def_id) { | |
if associated_body(Node::Item(item)).is_some() { |
0366bfc
to
327a469
Compare
This comment has been minimized.
This comment has been minimized.
8343258
to
b535db3
Compare
This comment has been minimized.
This comment has been minimized.
b535db3
to
1160496
Compare
This comment has been minimized.
This comment has been minimized.
if matches!(ex.kind, ExprKind::Closure { .. }) | ||
&& associated_body(Node::Expr(ex)).is_some() | ||
{ | ||
self.body_owners.push(ex.hir_id.owner); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We want to record the LocalDefId
of the closure. hir_id.owner
is the one of the closure's surrounding item-like (that we have already recorded in visit_*item
).
self.body_owners.push(ex.hir_id.owner); | |
self.body_owners.push(self.tcx.hir().local_def_id(ex.hir_id)); |
Likewise for anon consts.
self.items.push(item.item_id()); | ||
|
||
if self.tcx.hir().is_body_owner(item.def_id) { | ||
self.body_owners.push(item.def_id); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This probably pushes the same LocalDefId
twice.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, that was the problem.
bbc27a9
to
17734a6
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for the delay @kckeiks, I had not realized you were waiting for me.
} | ||
|
||
fn visit_anon_const(&mut self, c: &'hir AnonConst) { | ||
if associated_body(Node::AnonConst(c)).is_some() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is always true.
|
||
fn visit_expr(&mut self, ex: &'hir Expr<'hir>) { | ||
if matches!(ex.kind, ExprKind::Closure { .. }) | ||
&& associated_body(Node::Expr(ex)).is_some() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
associated_body(...).is_some()
is always true on closures.
self.impl_items.push(item.impl_item_id()); | ||
intravisit::walk_impl_item(self, item) | ||
} | ||
|
||
fn visit_foreign_item(&mut self, item: &'hir ForeignItem<'hir>) { | ||
if associated_body(Node::ForeignItem(item)).is_some() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is always false.
self.trait_items.push(item.trait_item_id()); | ||
intravisit::walk_trait_item(self, item) | ||
} | ||
|
||
fn visit_impl_item(&mut self, item: &'hir ImplItem<'hir>) { | ||
if associated_body(Node::ImplItem(item)).is_some() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you inline the definition of associated_body
here?
@@ -1281,19 +1291,47 @@ pub(super) fn hir_module_items(tcx: TyCtxt<'_>, module_id: LocalDefId) -> Module | |||
} | |||
|
|||
fn visit_trait_item(&mut self, item: &'hir TraitItem<'hir>) { | |||
if associated_body(Node::TraitItem(item)).is_some() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you inline the definition of associated_body
here?
@@ -1271,7 +1276,12 @@ pub(super) fn hir_module_items(tcx: TyCtxt<'_>, module_id: LocalDefId) -> Module | |||
} | |||
|
|||
fn visit_item(&mut self, item: &'hir Item<'hir>) { | |||
if associated_body(Node::Item(item)).is_some() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you inline the definition of associated_body
here?
@@ -1327,6 +1374,7 @@ pub(crate) fn hir_crate_items(tcx: TyCtxt<'_>, _: ()) -> ModuleItems { | |||
trait_items: Vec<TraitItemId>, | |||
impl_items: Vec<ImplItemId>, | |||
foreign_items: Vec<ForeignItemId>, | |||
body_owners: Vec<LocalDefId>, | |||
} | |||
|
|||
impl<'hir> Visitor<'hir> for CrateCollector<'hir> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if we can merge this visitor and the one for hir_module_items
using some flag to switch behaviour.
@@ -464,6 +464,23 @@ impl<'hir> Map<'hir> { | |||
} | |||
} | |||
|
|||
/// Returns true if the `LocalDefId` corresponds to a body owner and false otherwise. | |||
pub fn is_body_owner(self, def_id: LocalDefId) -> bool { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this used anywhere?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, I forgot to remove this.
| DefKind::AnonConst | ||
| DefKind::Ctor(..) | ||
| DefKind::Fn | ||
| DefKind::AssocFn |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This case is possibly wrong.
| DefKind::AssocFn | ||
| DefKind::Closure | ||
| DefKind::Generator | ||
| DefKind::Static(..) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This one too.
Signed-off-by: Miguel Guarniz <mi9uel9@gmail.com>
Signed-off-by: Miguel Guarniz <mi9uel9@gmail.com>
Signed-off-by: Miguel Guarniz <mi9uel9@gmail.com>
4d38f6e
to
75ffbb0
Compare
This comment has been minimized.
This comment has been minimized.
@bors retry |
⌛ Testing commit 75ffbb021cc96d6032fe5cad46153e401dc51802 with merge adc213b9b01134a4bcfc3efdac1e293946e6ab2d... |
💔 Test failed - checks-actions |
This comment has been minimized.
This comment has been minimized.
} | ||
}); | ||
|
||
par_iter(self.tcx.hir_crate_items(()).body_owners.to_vec()).for_each(|def_id| f(def_id)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
par_iter(self.tcx.hir_crate_items(()).body_owners.to_vec()).for_each(|def_id| f(def_id)); | |
par_iter(&self.tcx.hir_crate_items(()).body_owners[..]).for_each(|&def_id| f(def_id)); |
?
self.submodules.push(n.owner); | ||
intravisit::walk_mod(self, m, n); | ||
} | ||
fn visit_mod(&mut self, m: &'hir Mod<'hir>, _s: Span, n: HirId) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will insert a module as its own submodule. We should probably handle all ItemKind::Mod
in visit_item
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the if-else in visit_item
would prevent that from happening? visit_mod
gets called only by walk_item
.
if !self.crate_collector && let ItemKind::Mod(..) = item.kind {
// If this declares another module, do not recurse inside it.
self.submodules.push(item.def_id);
} else {
intravisit::walk_item(self, item)
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
visit_mod
is the entry point called by hir_module_items
.
Signed-off-by: Miguel Guarniz <mi9uel9@gmail.com>
Signed-off-by: Miguel Guarniz <mi9uel9@gmail.com>
Signed-off-by: Miguel Guarniz <mi9uel9@gmail.com>
75ffbb0
to
ad76362
Compare
Signed-off-by: Miguel Guarniz <mi9uel9@gmail.com>
ad76362
to
2d265b6
Compare
@bors r+ |
☀️ Test successful - checks-actions |
Finished benchmarking commit (30243dd): comparison url. Instruction count
Max RSS (memory usage)Results
CyclesResults
If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf. @rustbot label: -perf-regression Footnotes |
Issue #96341