-
Notifications
You must be signed in to change notification settings - Fork 12.9k
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
Convert Dependency Graph Tasks from in_task
s to with_task
s
#40376
Conversation
A task function is now given as a `fn` pointer to ensure that it carries no state. Each fn can take two arguments, because that worked out to be convenient -- these two arguments must be of some type that is `DepGraphSafe`, a new trait that is intended to prevent "leaking" information into the task that was derived from tracked state. This intentionally leaves `DepGraph::in_task()`, the more common form, alone. Eventually all uses of `DepGraph::in_task()` should be ported to `with_task()`, but I wanted to start with a smaller subset. Originally I wanted to use closures bound by an auto trait, but that approach has some limitations: - the trait cannot have a `read()` method; since the current method is unused, that may not be a problem. - more importantly, we would want the auto trait to be "undefined" for all types *by default* -- that is, this use case doesn't really fit the typical auto trait scenario. For example, imagine that there is a `u32` loaded out of a `hir::Node` -- we don't really want to be passing that `u32` into the task!
Can we wait until the next release, so we can use closure to |
@eddyb That'd be a lot prettier, haha |
It doesn't seem that different? we'd delete a line or two per case, basically, but we'd still wind up re-indenting and everything. |
Anyway by the time we land this it'll probably be the next release anyway... |
☔ The latest upstream changes (presumably #40308) made this pull request unmergeable. Please resolve the merge conflicts. |
So @cramertj should we close this in favor of the on-demandification approach? |
@nikomatsakis Sure. |
r? @nikomatsakis