Replace uses of DepGraph.in_ignore with DepGraph.with_ignore #47087
Conversation
| tcx.dep_graph.with_ignore(|| { | ||
| let mut visitor = SymbolNamesTest { tcx: tcx }; | ||
| // FIXME(#37712) could use ItemLikeVisitor if trait items were item-like | ||
| tcx.hir.krate().visit_all_item_likes(&mut visitor.as_deep_visitor()); |
There was a problem hiding this comment.
[00:04:05] tidy error: /checkout/src/librustc_trans/symbol_names_test.rs:36: tab character
[00:04:05] tidy error: /checkout/src/librustc_trans/symbol_names_test.rs:37: tab character
[00:04:05] tidy error: /checkout/src/librustc_trans/symbol_names_test.rs:38: tab character
kennytm
added
the
S-waiting-on-review
Zoxc
force-pushed
the
incr_no_in_ignore
branch
from
ef2719a to
383fb42
Compare
There was a problem hiding this comment.
Thanks for the PR, @Zoxc! We wanted to get rid of in_ignore() for a while.
I would prefer it though to just use with_ignore() instead of assert_ignored(). Less opportunity to get something wrong. And remove in_ignore() completely if possible.
|
I do not want to create any |
|
I'm wondering if we can instead move cc @eddyb |
|
Maybe, yeah. |
src/librustc/dep_graph/graph.rs
Outdated
| if let Some(ref data) = self.data { | ||
| match data.current.borrow_mut().task_stack.last_mut() { | ||
| Some(&mut OpenTask::Ignore) | | ||
| Some(&mut OpenTask::EvalAlways { .. }) | None => { |
There was a problem hiding this comment.
EvalAlways is conceptually not ignored. I'd remove this case.
There was a problem hiding this comment.
Also, you should be able to do non-mutable access here, right?
There was a problem hiding this comment.
I copied the ignored cases from read_index. Why is EvalAlways ignored there then?
There was a problem hiding this comment.
Because we artificial add a single edge to DepNode::Krate when the task finishes. This single edge will detect any change made to the crate or any upstream crates.
kennytm
added
S-waiting-on-author
Zoxc
force-pushed
the
incr_no_in_ignore
branch
from
383fb42 to
9508922
Compare
|
@michaelwoerister I addressed your comments |
kennytm
added
S-waiting-on-review
|
📌 Commit 9508922 has been approved by |
kennytm
added
S-waiting-on-bors
|
⌛ Testing commit 9508922 with merge 3686fc73958238f97ba6d75735017e14a7c6cbc8... |
|
💔 Test failed - status-appveyor |
|
@rust-lang/infra, is this AppVeyor being slow at the moment? |
Replace uses of DepGraph.in_ignore with DepGraph.with_ignore I currently plan to track tasks in thread local storage. Ignoring things in a closure ensures that the ignore tasks do not overlap the beginning or end of any other task. The TLS API will also use a closure to change a TLS value, so having the ignore task be a closure also helps there. It also adds `assert_ignored` which is used before a `TyCtxt` is created. Instead of adding a new ignore task this simply ensures that we are in a context where reads are ignored. r? @michaelwoerister
|
☀️ Test successful - status-appveyor, status-travis |
I currently plan to track tasks in thread local storage. Ignoring things in a closure ensures that the ignore tasks do not overlap the beginning or end of any other task. The TLS API will also use a closure to change a TLS value, so having the ignore task be a closure also helps there.
It also adds
assert_ignoredwhich is used before aTyCtxtis created. Instead of adding a new ignore task this simply ensures that we are in a context where reads are ignored.r? @michaelwoerister