std: Remove RefCell::get()

erickt · erickt · commit 7bcfe2ee1067 · 2014-04-03T20:28:55.000-07:00
It's surprising that `RefCell::get()` is implicitly doing a clone
on a value. This patch removes it and replaces all users with
either `.borrow()` when we can autoderef, or `.borrow().clone()`
when we cannot.
diff --git a/src/librustc/front/test.rs b/src/librustc/front/test.rs
@@ -90,7 +90,7 @@ impl<'a> fold::Folder for TestHarnessGenerator<'a> {
     fn fold_item(&mut self, i: @ast::Item) -> SmallVector<@ast::Item> {
         self.cx.path.borrow_mut().push(i.ident);
         debug!("current path: {}",
-               ast_util::path_name_i(self.cx.path.get().as_slice()));
+               ast_util::path_name_i(self.cx.path.borrow().as_slice()));
 
         if is_test_fn(&self.cx, i) || is_bench_fn(&self.cx, i) {
             match i.node {
@@ -104,7 +104,7 @@ impl<'a> fold::Folder for TestHarnessGenerator<'a> {
                     debug!("this is a test function");
                     let test = Test {
                         span: i.span,
-                        path: self.cx.path.get(),
+                        path: self.cx.path.borrow().clone(),
                         bench: is_bench_fn(&self.cx, i),
                         ignore: is_ignored(&self.cx, i),
                         should_fail: should_fail(i)
diff --git a/src/librustc/metadata/encoder.rs b/src/librustc/metadata/encoder.rs
@@ -1345,7 +1345,7 @@ fn encode_info_for_items(ecx: &EncodeContext,
     }
 
     ebml_w.end_tag();
-    return /*bad*/(*index).get();
+    return /*bad*/index.borrow().clone();
 }
 
 
@@ -1365,7 +1365,7 @@ fn create_index<T:Clone + Hash + 'static>(
 
     let mut buckets_frozen = Vec::new();
     for bucket in buckets.iter() {
-        buckets_frozen.push(@/*bad*/(**bucket).get());
+        buckets_frozen.push(@/*bad*/bucket.borrow().clone());
     }
     return buckets_frozen;
 }
diff --git a/src/librustc/middle/dead.rs b/src/librustc/middle/dead.rs
@@ -270,7 +270,7 @@ fn create_and_seed_worklist(tcx: &ty::ctxt,
     }
 
     // Seed entry point
-    match tcx.sess.entry_fn.get() {
+    match *tcx.sess.entry_fn.borrow() {
         Some((id, _)) => worklist.push(id),
         None => ()
     }
diff --git a/src/librustc/middle/resolve.rs b/src/librustc/middle/resolve.rs
@@ -385,6 +385,10 @@ struct ImportResolution {
     type_id: Cell<NodeId>,
 }
 
+fn get<T: Clone>(cell: &RefCell<T>) -> T {
+    cell.borrow().clone()
+}
+
 impl ImportResolution {
     fn new(id: NodeId, is_public: bool) -> ImportResolution {
         ImportResolution {
@@ -400,8 +404,8 @@ impl ImportResolution {
     fn target_for_namespace(&self, namespace: Namespace)
                                 -> Option<Target> {
         match namespace {
-            TypeNS      => return self.type_target.get(),
-            ValueNS     => return self.value_target.get(),
+            TypeNS      => return self.type_target.borrow().clone(),
+            ValueNS     => return self.value_target.borrow().clone(),
         }
     }
 
@@ -546,7 +550,8 @@ impl NameBindings {
         // Merges the module with the existing type def or creates a new one.
         let module_ = @Module::new(parent_link, def_id, kind, external,
                                        is_public);
-        match self.type_def.get() {
+        let type_def = self.type_def.borrow().clone();
+        match type_def {
             None => {
                 self.type_def.set(Some(TypeNsDef {
                     is_public: is_public,
@@ -574,7 +579,8 @@ impl NameBindings {
                        external: bool,
                        is_public: bool,
                        _sp: Span) {
-        match self.type_def.get() {
+        let type_def = self.type_def.borrow().clone();
+        match type_def {
             None => {
                 let module = @Module::new(parent_link, def_id, kind,
                                               external, is_public);
@@ -609,7 +615,8 @@ impl NameBindings {
     /// Records a type definition.
     fn define_type(&self, def: Def, sp: Span, is_public: bool) {
         // Merges the type with the existing type def or creates a new one.
-        match self.type_def.get() {
+        let type_def = self.type_def.borrow().clone();
+        match type_def {
             None => {
                 self.type_def.set(Some(TypeNsDef {
                     module_def: None,
@@ -662,17 +669,17 @@ impl NameBindings {
 
     fn defined_in_namespace(&self, namespace: Namespace) -> bool {
         match namespace {
-            TypeNS   => return self.type_def.get().is_some(),
-            ValueNS  => return self.value_def.get().is_some()
+            TypeNS   => return self.type_def.borrow().is_some(),
+            ValueNS  => return self.value_def.borrow().is_some()
         }
     }
 
     fn defined_in_public_namespace(&self, namespace: Namespace) -> bool {
         match namespace {
-            TypeNS => match self.type_def.get() {
+            TypeNS => match *self.type_def.borrow() {
                 Some(def) => def.is_public, None => false
             },
-            ValueNS => match self.value_def.get() {
+            ValueNS => match *self.value_def.borrow() {
                 Some(def) => def.is_public, None => false
             }
         }
@@ -681,7 +688,7 @@ impl NameBindings {
     fn def_for_namespace(&self, namespace: Namespace) -> Option<Def> {
         match namespace {
             TypeNS => {
-                match self.type_def.get() {
+                match *self.type_def.borrow() {
                     None => None,
                     Some(type_def) => {
                         match type_def.type_def {
@@ -702,7 +709,7 @@ impl NameBindings {
                 }
             }
             ValueNS => {
-                match self.value_def.get() {
+                match *self.value_def.borrow() {
                     None => None,
                     Some(value_def) => Some(value_def.def)
                 }
@@ -714,13 +721,13 @@ impl NameBindings {
         if self.defined_in_namespace(namespace) {
             match namespace {
                 TypeNS  => {
-                    match self.type_def.get() {
+                    match *self.type_def.borrow() {
                         None => None,
                         Some(type_def) => type_def.type_span
                     }
                 }
                 ValueNS => {
-                    match self.value_def.get() {
+                    match *self.value_def.borrow() {
                         None => None,
                         Some(value_def) => value_def.value_span
                     }
@@ -1620,7 +1627,8 @@ impl<'a> Resolver<'a> {
         match def {
           DefMod(def_id) | DefForeignMod(def_id) | DefStruct(def_id) |
           DefTy(def_id) => {
-            match child_name_bindings.type_def.get() {
+            let type_def = child_name_bindings.type_def.borrow().clone();
+            match type_def {
               Some(TypeNsDef { module_def: Some(module_def), .. }) => {
                 debug!("(building reduced graph for external crate) \
                         already created module");
@@ -1812,7 +1820,8 @@ impl<'a> Resolver<'a> {
                                 // Process the static methods. First,
                                 // create the module.
                                 let type_module;
-                                match child_name_bindings.type_def.get() {
+                                let type_def = child_name_bindings.type_def.borrow().clone();
+                                match type_def {
                                     Some(TypeNsDef {
                                         module_def: Some(module_def),
                                         ..
@@ -2421,7 +2430,7 @@ impl<'a> Resolver<'a> {
         match type_result {
             BoundResult(target_module, name_bindings) => {
                 debug!("(resolving single import) found type target: {:?}",
-                       {name_bindings.type_def.get().unwrap().type_def});
+                       { name_bindings.type_def.borrow().clone().unwrap().type_def });
                 import_resolution.type_target.set(
                     Some(Target::new(target_module, name_bindings)));
                 import_resolution.type_id.set(directive.id);
@@ -2433,8 +2442,8 @@ impl<'a> Resolver<'a> {
             }
         }
 
-        if import_resolution.value_target.get().is_none() &&
-           import_resolution.type_target.get().is_none() {
+        if import_resolution.value_target.borrow().is_none() &&
+           import_resolution.type_target.borrow().is_none() {
             let msg = format!("unresolved import: there is no \
                                `{}` in `{}`",
                               token::get_ident(source),
@@ -2452,7 +2461,7 @@ impl<'a> Resolver<'a> {
         // record what this import resolves to for later uses in documentation,
         // this may resolve to either a value or a type, but for documentation
         // purposes it's good enough to just favor one over the other.
-        let value_private = match import_resolution.value_target.get() {
+        let value_private = match *import_resolution.value_target.borrow() {
             Some(target) => {
                 let def = target.bindings.def_for_namespace(ValueNS).unwrap();
                 self.def_map.borrow_mut().insert(directive.id, def);
@@ -2463,7 +2472,7 @@ impl<'a> Resolver<'a> {
             // _exists is false.
             None => None,
         };
-        let type_private = match import_resolution.type_target.get() {
+        let type_private = match *import_resolution.type_target.borrow() {
             Some(target) => {
                 let def = target.bindings.def_for_namespace(TypeNS).unwrap();
                 self.def_map.borrow_mut().insert(directive.id, def);
@@ -2513,7 +2522,7 @@ impl<'a> Resolver<'a> {
         for (ident, target_import_resolution) in import_resolutions.iter() {
             debug!("(resolving glob import) writing module resolution \
                     {:?} into `{}`",
-                   target_import_resolution.type_target.get().is_none(),
+                   target_import_resolution.type_target.borrow().is_none(),
                    self.module_to_str(module_));
 
             if !target_import_resolution.is_public.get() {
@@ -2529,9 +2538,9 @@ impl<'a> Resolver<'a> {
                     let new_import_resolution =
                         @ImportResolution::new(id, is_public);
                     new_import_resolution.value_target.set(
-                        target_import_resolution.value_target.get());
+                        get(&target_import_resolution.value_target));
                     new_import_resolution.type_target.set(
-                        target_import_resolution.type_target.get());
+                        get(&target_import_resolution.type_target));
 
                     import_resolutions.insert
                         (*ident, new_import_resolution);
@@ -2540,7 +2549,7 @@ impl<'a> Resolver<'a> {
                     // Merge the two import resolutions at a finer-grained
                     // level.
 
-                    match target_import_resolution.value_target.get() {
+                    match *target_import_resolution.value_target.borrow() {
                         None => {
                             // Continue.
                         }
@@ -2549,7 +2558,7 @@ impl<'a> Resolver<'a> {
                                 Some(value_target));
                         }
                     }
-                    match target_import_resolution.type_target.get() {
+                    match *target_import_resolution.type_target.borrow() {
                         None => {
                             // Continue.
                         }
@@ -2692,7 +2701,7 @@ impl<'a> Resolver<'a> {
                 Success((target, used_proxy)) => {
                     // Check to see whether there are type bindings, and, if
                     // so, whether there is a module within.
-                    match target.bindings.type_def.get() {
+                    match *target.bindings.type_def.borrow() {
                         Some(type_def) => {
                             match type_def.module_def {
                                 None => {
@@ -3004,7 +3013,7 @@ impl<'a> Resolver<'a> {
         match resolve_result {
             Success((target, _)) => {
                 let bindings = &*target.bindings;
-                match bindings.type_def.get() {
+                match *bindings.type_def.borrow() {
                     Some(type_def) => {
                         match type_def.module_def {
                             None => {
@@ -4526,8 +4535,8 @@ impl<'a> Resolver<'a> {
                 debug!("(resolve bare identifier pattern) succeeded in \
                          finding {} at {:?}",
                         token::get_ident(name),
-                        target.bindings.value_def.get());
-                match target.bindings.value_def.get() {
+                        target.bindings.value_def.borrow());
+                match *target.bindings.value_def.borrow() {
                     None => {
                         fail!("resolved name in the value namespace to a \
                               set of name bindings with no def?!");
diff --git a/src/librustc/middle/trans/base.rs b/src/librustc/middle/trans/base.rs
@@ -1128,7 +1128,7 @@ pub fn make_return_pointer(fcx: &FunctionContext, output_type: ty::t)
             llvm::LLVMGetParam(fcx.llfn, 0)
         } else {
             let lloutputtype = type_of::type_of(fcx.ccx, output_type);
-            let bcx = fcx.entry_bcx.get().unwrap();
+            let bcx = fcx.entry_bcx.borrow().clone().unwrap();
             Alloca(bcx, lloutputtype, "__make_return_pointer")
         }
     }
@@ -1399,7 +1399,7 @@ pub fn trans_closure(ccx: &CrateContext,
 
     // Create the first basic block in the function and keep a handle on it to
     //  pass to finish_fn later.
-    let bcx_top = fcx.entry_bcx.get().unwrap();
+    let bcx_top = fcx.entry_bcx.borrow().clone().unwrap();
     let mut bcx = bcx_top;
     let block_ty = node_id_type(bcx, body.id);
 
@@ -1547,7 +1547,7 @@ fn trans_enum_variant_or_tuple_like_struct(ccx: &CrateContext,
 
     let arg_datums = create_datums_for_fn_args(&fcx, arg_tys.as_slice());
 
-    let bcx = fcx.entry_bcx.get().unwrap();
+    let bcx = fcx.entry_bcx.borrow().clone().unwrap();
 
     if !type_is_zero_size(fcx.ccx, result_ty) {
         let repr = adt::represent_type(ccx, result_ty);
@@ -1752,7 +1752,7 @@ pub fn register_fn_llvmty(ccx: &CrateContext,
 }
 
 pub fn is_entry_fn(sess: &Session, node_id: ast::NodeId) -> bool {
-    match sess.entry_fn.get() {
+    match *sess.entry_fn.borrow() {
         Some((entry_id, _)) => node_id == entry_id,
         None => false
     }
diff --git a/src/librustc/middle/trans/closure.rs b/src/librustc/middle/trans/closure.rs
@@ -464,7 +464,7 @@ pub fn get_wrapper_for_bare_fn(ccx: &CrateContext,
     let arena = TypedArena::new();
     let fcx = new_fn_ctxt(ccx, llfn, -1, true, f.sig.output, None, None, &arena);
     init_function(&fcx, true, f.sig.output, None);
-    let bcx = fcx.entry_bcx.get().unwrap();
+    let bcx = fcx.entry_bcx.borrow().clone().unwrap();
 
     let args = create_datums_for_fn_args(&fcx,
                                          ty::ty_fn_args(closure_ty)
diff --git a/src/librustc/middle/trans/glue.rs b/src/librustc/middle/trans/glue.rs
@@ -463,7 +463,7 @@ fn make_generic_glue(ccx: &CrateContext,
     // llfn is expected be declared to take a parameter of the appropriate
     // type, so we don't need to explicitly cast the function parameter.
 
-    let bcx = fcx.entry_bcx.get().unwrap();
+    let bcx = fcx.entry_bcx.borrow().clone().unwrap();
     let llrawptr0 = unsafe { llvm::LLVMGetParam(llfn, fcx.arg_pos(0) as c_uint) };
     let bcx = helper(bcx, llrawptr0, t);
     finish_fn(&fcx, bcx);
diff --git a/src/librustc/middle/trans/intrinsic.rs b/src/librustc/middle/trans/intrinsic.rs
@@ -199,7 +199,7 @@ pub fn trans_intrinsic(ccx: &CrateContext,
 
     set_always_inline(fcx.llfn);
 
-    let mut bcx = fcx.entry_bcx.get().unwrap();
+    let mut bcx = fcx.entry_bcx.borrow().clone().unwrap();
     let first_real_arg = fcx.arg_pos(0u);
 
     let name = token::get_ident(item.ident);
diff --git a/src/librustc/middle/trans/reflect.rs b/src/librustc/middle/trans/reflect.rs
@@ -300,7 +300,7 @@ impl<'a> Reflector<'a> {
                     //
                     llvm::LLVMGetParam(llfdecl, fcx.arg_pos(0u) as c_uint)
                 };
-                let bcx = fcx.entry_bcx.get().unwrap();
+                let bcx = fcx.entry_bcx.borrow().clone().unwrap();
                 let arg = BitCast(bcx, arg, llptrty);
                 let ret = adt::trans_get_discr(bcx, repr, arg, Some(Type::i64(ccx)));
                 Store(bcx, ret, fcx.llretptr.get().unwrap());
diff --git a/src/librustc/middle/typeck/check/mod.rs b/src/librustc/middle/typeck/check/mod.rs
@@ -2209,8 +2209,8 @@ fn check_expr_with_unifier(fcx: &FnCtxt,
         fcx.write_ty(expr.id, fty);
 
         let (inherited_purity, id) =
-            ty::determine_inherited_purity((fcx.ps.get().purity,
-                                            fcx.ps.get().def),
+            ty::determine_inherited_purity((fcx.ps.borrow().purity,
+                                            fcx.ps.borrow().def),
                                            (purity, expr.id),
                                            sigil);
 
diff --git a/src/librustc/middle/typeck/check/regionck.rs b/src/librustc/middle/typeck/check/regionck.rs
@@ -398,7 +398,7 @@ fn visit_expr(rcx: &mut Rcx, expr: &ast::Expr) {
            expr.repr(rcx.fcx.tcx()), rcx.repeating_scope);
 
     let method_call = MethodCall::expr(expr.id);
-    let has_method_map = rcx.fcx.inh.method_map.get().contains_key(&method_call);
+    let has_method_map = rcx.fcx.inh.method_map.borrow().contains_key(&method_call);
 
     // Check any autoderefs or autorefs that appear.
     for &adjustment in rcx.fcx.inh.adjustments.borrow().find(&expr.id).iter() {
@@ -498,7 +498,7 @@ fn visit_expr(rcx: &mut Rcx, expr: &ast::Expr) {
         ast::ExprUnary(ast::UnDeref, base) => {
             // For *a, the lifetime of a must enclose the deref
             let method_call = MethodCall::expr(expr.id);
-            let base_ty = match rcx.fcx.inh.method_map.get().find(&method_call) {
+            let base_ty = match rcx.fcx.inh.method_map.borrow().find(&method_call) {
                 Some(method) => {
                     constrain_call(rcx, None, expr, Some(base), [], true);
                     ty::ty_fn_ret(method.ty)
@@ -852,7 +852,7 @@ fn constrain_autoderefs(rcx: &mut Rcx,
                i, derefs);
 
         let method_call = MethodCall::autoderef(deref_expr.id, i as u32);
-        derefd_ty = match rcx.fcx.inh.method_map.get().find(&method_call) {
+        derefd_ty = match rcx.fcx.inh.method_map.borrow().find(&method_call) {
             Some(method) => {
                 // Treat overloaded autoderefs as if an AutoRef adjustment
                 // was applied on the base type, as that is always the case.
diff --git a/src/librustc/middle/typeck/infer/error_reporting.rs b/src/librustc/middle/typeck/infer/error_reporting.rs
@@ -1286,6 +1286,6 @@ impl LifeGiver {
     }
 
     fn get_generated_lifetimes(&self) -> Vec<ast::Lifetime> {
-        self.generated.get()
+        self.generated.borrow().clone()
     }
 }
diff --git a/src/librustc/middle/typeck/mod.rs b/src/librustc/middle/typeck/mod.rs
diff --git a/src/libstd/cell.rs b/src/libstd/cell.rs
diff --git a/src/libstd/option.rs b/src/libstd/option.rs
diff --git a/src/test/auxiliary/issue-2631-a.rs b/src/test/auxiliary/issue-2631-a.rs

Original file line number	Diff line number	Diff line change
`@@ -1345,7 +1345,7 @@ fn encode_info_for_items(ecx: &EncodeContext,`
`1345`	`1345`	`}`
`1346`	`1346`
`1347`	`1347`	`ebml_w.end_tag();`
`1348`		`- return /bad/(*index).get();`
	`1348`	`+ return /bad/index.borrow().clone();`
`1349`	`1349`	`}`
`1350`	`1350`
`1351`	`1351`
`@@ -1365,7 +1365,7 @@ fn create_index<T:Clone + Hash + 'static>(`
`1365`	`1365`
`1366`	`1366`	`let mut buckets_frozen = Vec::new();`
`1367`	`1367`	`for bucket in buckets.iter() {`
`1368`		`- buckets_frozen.push(@/bad/(**bucket).get());`
	`1368`	`+ buckets_frozen.push(@/bad/bucket.borrow().clone());`
`1369`	`1369`	`}`
`1370`	`1370`	`return buckets_frozen;`
`1371`	`1371`	`}`
Original file line number	Diff line number	Diff line change
`@@ -270,7 +270,7 @@ fn create_and_seed_worklist(tcx: &ty::ctxt,`
`270`	`270`	`}`
`271`	`271`
`272`	`272`	`// Seed entry point`
`273`		`- match tcx.sess.entry_fn.get() {`
	`273`	`+ match *tcx.sess.entry_fn.borrow() {`
`274`	`274`	`Some((id, _)) => worklist.push(id),`
`275`	`275`	`None => ()`
`276`	`276`	`}`