From 4d5441fe3dc0869b3f6637e6320c9091b8d9efa0 Mon Sep 17 00:00:00 2001 From: Niko Matsakis Date: Wed, 8 Mar 2017 09:14:27 -0500 Subject: [PATCH] add comments and remove unused code paths --- src/librustc/dep_graph/graph.rs | 29 +++++++++++++++++-- src/librustc/dep_graph/mod.rs | 1 - src/librustc/dep_graph/safe.rs | 51 ++++++++++++++------------------- src/librustc/lib.rs | 1 - src/librustc_trans/context.rs | 2 -- 5 files changed, 49 insertions(+), 35 deletions(-) diff --git a/src/librustc/dep_graph/graph.rs b/src/librustc/dep_graph/graph.rs index e22f56d278d55..8be5d4327e72e 100644 --- a/src/librustc/dep_graph/graph.rs +++ b/src/librustc/dep_graph/graph.rs @@ -77,12 +77,37 @@ impl DepGraph { op() } + /// Starts a new dep-graph task. Dep-graph tasks are specified + /// using a free function (`task`) and **not** a closure -- this + /// is intentional because we want to exercise tight control over + /// what state they have access to. In particular, we want to + /// prevent implicit 'leaks' of tracked state into the task (which + /// could then be read without generating correct edges in the + /// dep-graph -- see the [README] for more details on the + /// dep-graph). To this end, the task function gets exactly two + /// pieces of state: the context `cx` and an argument `arg`. Both + /// of these bits of state must be of some type that implements + /// `DepGraphSafe` and hence does not leak. + /// + /// The choice of two arguments is not fundamental. One argument + /// would work just as well, since multiple values can be + /// collected using tuples. However, using two arguments works out + /// to be quite convenient, since it is common to need a context + /// (`cx`) and some argument (e.g., a `DefId` identifying what + /// item to process). + /// + /// For cases where you need some other number of arguments: + /// + /// - If you only need one argument, just use `()` for the `arg` + /// parameter. + /// - If you need 3+ arguments, use a tuple for the + /// `arg` parameter. + /// + /// [README]: README.md pub fn with_task(&self, key: DepNode, cx: C, arg: A, task: fn(C, A) -> R) -> R where C: DepGraphSafe, A: DepGraphSafe { let _task = self.in_task(key); - cx.read(self); - arg.read(self); task(cx, arg) } diff --git a/src/librustc/dep_graph/mod.rs b/src/librustc/dep_graph/mod.rs index 496375b129167..a9f0a44e4208c 100644 --- a/src/librustc/dep_graph/mod.rs +++ b/src/librustc/dep_graph/mod.rs @@ -15,7 +15,6 @@ mod edges; mod graph; mod query; mod raii; -#[macro_use] mod safe; mod shadow; mod thread; diff --git a/src/librustc/dep_graph/safe.rs b/src/librustc/dep_graph/safe.rs index 9c5b110929e22..f85f0338ed997 100644 --- a/src/librustc/dep_graph/safe.rs +++ b/src/librustc/dep_graph/safe.rs @@ -13,58 +13,51 @@ use hir::def_id::DefId; use syntax::ast::NodeId; use ty::TyCtxt; -use super::graph::DepGraph; - -/// The `DepGraphSafe` auto trait is used to specify what kinds of -/// values are safe to "leak" into a task. The idea is that this -/// should be only be implemented for things like the tcx, which will -/// create reads in the dep-graph whenever the trait loads anything -/// that might depend on the input program. +/// The `DepGraphSafe` trait is used to specify what kinds of values +/// are safe to "leak" into a task. The idea is that this should be +/// only be implemented for things like the tcx as well as various id +/// types, which will create reads in the dep-graph whenever the trait +/// loads anything that might depend on the input program. pub trait DepGraphSafe { - fn read(&self, graph: &DepGraph); } +/// A `BodyId` on its own doesn't give access to any particular state. +/// You must fetch the state from the various maps or generate +/// on-demand queries, all of which create reads. impl DepGraphSafe for BodyId { - fn read(&self, _graph: &DepGraph) { - // a BodyId on its own doesn't give access to any particular state - } } +/// A `NodeId` on its own doesn't give access to any particular state. +/// You must fetch the state from the various maps or generate +/// on-demand queries, all of which create reads. impl DepGraphSafe for NodeId { - fn read(&self, _graph: &DepGraph) { - // a DefId doesn't give any particular state - } } +/// A `DefId` on its own doesn't give access to any particular state. +/// You must fetch the state from the various maps or generate +/// on-demand queries, all of which create reads. impl DepGraphSafe for DefId { - fn read(&self, _graph: &DepGraph) { - // a DefId doesn't give any particular state - } } +/// The type context itself can be used to access all kinds of tracked +/// state, but those accesses should always generate read events. impl<'a, 'gcx, 'tcx> DepGraphSafe for TyCtxt<'a, 'gcx, 'tcx> { - fn read(&self, _graph: &DepGraph) { - } } +/// Tuples make it easy to build up state. impl DepGraphSafe for (A, B) where A: DepGraphSafe, B: DepGraphSafe { - fn read(&self, graph: &DepGraph) { - self.0.read(graph); - self.1.read(graph); - } } +/// No data here! :) impl DepGraphSafe for () { - fn read(&self, _graph: &DepGraph) { - } } -/// A convenient override. We should phase out usage of this over -/// time. +/// A convenient override that lets you pass arbitrary state into a +/// task. Every use should be accompanied by a comment explaining why +/// it makes sense (or how it could be refactored away in the future). pub struct AssertDepGraphSafe(pub T); + impl DepGraphSafe for AssertDepGraphSafe { - fn read(&self, _graph: &DepGraph) { - } } diff --git a/src/librustc/lib.rs b/src/librustc/lib.rs index 2ba4054ef3f6d..c4fccdcb9eb62 100644 --- a/src/librustc/lib.rs +++ b/src/librustc/lib.rs @@ -70,7 +70,6 @@ mod macros; pub mod diagnostics; pub mod cfg; -#[macro_use] pub mod dep_graph; pub mod hir; pub mod infer; diff --git a/src/librustc_trans/context.rs b/src/librustc_trans/context.rs index f3aa3c67831ed..52851ea995d4b 100644 --- a/src/librustc_trans/context.rs +++ b/src/librustc_trans/context.rs @@ -276,8 +276,6 @@ pub struct CrateContext<'a, 'tcx: 'a> { } impl<'a, 'tcx> DepGraphSafe for CrateContext<'a, 'tcx> { - fn read(&self, _graph: &DepGraph) { - } } pub struct CrateContextIterator<'a, 'tcx: 'a> {