collector-driven-trans: Take care of nits.

Author: michaelwoerister

src/librustc_trans/base.rs

@@ -2667,9 +2667,10 @@ pub fn trans_crate<'a, 'tcx>(tcx: TyCtxt<'a, 'tcx, 'tcx>,
     {
         let ccx = crate_context_list.get_ccx(0);
 
+        // FIXME: #34018
         // At this point, we only walk the HIR for running
         // enum_variant_size_lint(). This should arguably be moved somewhere
-        // else
+        // else.
         {
             intravisit::walk_mod(&mut TransItemsWithinModVisitor { ccx: &ccx }, &krate.module);
             krate.visit_all_items(&mut TransModVisitor { ccx: &ccx });

src/librustc_trans/callee.rs

@@ -537,20 +537,15 @@ fn get_fn<'a, 'tcx>(ccx: &CrateContext<'a, 'tcx>,
     // reference. It also occurs when testing libcore and in some
     // other weird situations. Annoying.
 
-    // Let's see if we can get the symbol name from the symbol_map, so we don't
-    // have to recompute it.
-    let mut sym_data = String::new();
-    let sym = ccx.symbol_map().get(TransItem::Fn(instance)).unwrap_or_else(|| {
-        sym_data = instance.symbol_name(ccx.shared());
-        &sym_data[..]
-    });
+    let sym = ccx.symbol_map().get_or_compute(ccx.shared(),
+                                              TransItem::Fn(instance));
 
     let llptrty = type_of::type_of(ccx, fn_ptr_ty);
-    let llfn = if let Some(llfn) = declare::get_declared_value(ccx, sym) {
+    let llfn = if let Some(llfn) = declare::get_declared_value(ccx, &sym) {
         if let Some(span) = local_item {
-            if declare::get_defined_value(ccx, sym).is_some() {
+            if declare::get_defined_value(ccx, &sym).is_some() {
                 ccx.sess().span_fatal(span,
-                    &format!("symbol `{}` is already defined", sym));
+                    &format!("symbol `{}` is already defined", &sym));
             }
         }
 
@@ -566,7 +561,7 @@ fn get_fn<'a, 'tcx>(ccx: &CrateContext<'a, 'tcx>,
             llfn
         }
     } else {
-        let llfn = declare::declare_fn(ccx, sym, ty);
+        let llfn = declare::declare_fn(ccx, &sym, ty);
         assert_eq!(common::val_ty(llfn), llptrty);
         debug!("get_fn: not casting pointer!");
 

src/librustc_trans/glue.rs

@@ -235,7 +235,8 @@ fn get_drop_glue_core<'a, 'tcx>(ccx: &CrateContext<'a, 'tcx>,
     let g = g.map_ty(|t| get_drop_glue_type(ccx.tcx(), t));
     match ccx.drop_glues().borrow().get(&g) {
         Some(&(glue, _)) => glue,
-        None => { bug!("Could not find drop glue for {:?} -- {} -- {}",
+        None => { bug!("Could not find drop glue for {:?} -- {} -- {}. \
+                        It should have be instantiated during the pre-definition phase",
                        g,
                        TransItem::DropGlue(g).to_raw_string(),
                        ccx.codegen_unit().name) }

src/librustc_trans/monomorphize.rs

@@ -84,19 +84,14 @@ pub fn monomorphic_fn<'a, 'tcx>(ccx: &CrateContext<'a, 'tcx>,
         monomorphizing.insert(fn_id, depth + 1);
     }
 
-    // Let's see if we can get the symbol name from the symbol_map, so we don't
-    // have to recompute it.
-    let mut sym_data = String::new();
-    let symbol = ccx.symbol_map().get(TransItem::Fn(instance)).unwrap_or_else(|| {
-        sym_data = instance.symbol_name(ccx.shared());
-        &sym_data[..]
-    });
+    let symbol = ccx.symbol_map().get_or_compute(ccx.shared(),
+                                                 TransItem::Fn(instance));
 
-    debug!("monomorphize_fn mangled to {}", symbol);
-    assert!(declare::get_defined_value(ccx, symbol).is_none());
+    debug!("monomorphize_fn mangled to {}", &symbol);
+    assert!(declare::get_defined_value(ccx, &symbol).is_none());
 
     // FIXME(nagisa): perhaps needs a more fine grained selection?
-    let lldecl = declare::define_internal_fn(ccx, symbol, mono_ty);
+    let lldecl = declare::define_internal_fn(ccx, &symbol, mono_ty);
     // FIXME(eddyb) Doubt all extern fn should allow unwinding.
     attributes::unwind(lldecl, true);
 

src/librustc_trans/partitioning.rs

@@ -154,8 +154,7 @@ impl<'tcx> CodegenUnit<'tcx> {
 
         // The codegen tests rely on items being process in the same order as
         // they appear in the file, so for local items, we sort by node_id first
-        items.as_mut_slice().sort_by(|&(trans_item1, _), &(trans_item2, _)| {
-
+        items.sort_by(|&(trans_item1, _), &(trans_item2, _)| {
             let node_id1 = local_node_id(tcx, trans_item1);
             let node_id2 = local_node_id(tcx, trans_item2);
 
@@ -165,6 +164,7 @@ impl<'tcx> CodegenUnit<'tcx> {
                     let symbol_name2 = symbol_map.get(trans_item2).unwrap();
                     symbol_name1.cmp(symbol_name2)
                 }
+                // In the following two cases we can avoid looking up the symbol
                 (None, Some(_)) => Ordering::Less,
                 (Some(_), None) => Ordering::Greater,
                 (Some(node_id1), Some(node_id2)) => {
@@ -241,7 +241,7 @@ pub fn partition<'a, 'tcx, I>(tcx: TyCtxt<'a, 'tcx, 'tcx>,
 
     // Finally, sort by codegen unit name, so that we get deterministic results
     let mut result = post_inlining.0;
-    result.as_mut_slice().sort_by(|cgu1, cgu2| {
+    result.sort_by(|cgu1, cgu2| {
         (&cgu1.name[..]).cmp(&cgu2.name[..])
     });
 
@@ -348,7 +348,7 @@ fn merge_codegen_units<'tcx>(initial_partitioning: &mut PreInliningPartitioning<
     // translation items in a given unit. This could be improved on.
     while codegen_units.len() > target_cgu_count {
         // Sort small cgus to the back
-        codegen_units.as_mut_slice().sort_by_key(|cgu| -(cgu.items.len() as i64));
+        codegen_units.sort_by_key(|cgu| -(cgu.items.len() as i64));
         let smallest = codegen_units.pop().unwrap();
         let second_smallest = codegen_units.last_mut().unwrap();
 

src/librustc_trans/symbol_map.rs

@@ -11,13 +11,15 @@
 use context::SharedCrateContext;
 use monomorphize::Instance;
 use rustc::ty::TyCtxt;
+use std::borrow::Cow;
 use syntax::codemap::Span;
 use trans_item::TransItem;
 use util::nodemap::FnvHashMap;
 
-
 // In the SymbolMap we collect the symbol names of all translation items of
-// the current crate.
+// the current crate. This map exists as a performance optimization. Symbol
+// names of translation items are deterministic and fully defined by the item.
+// Thus they could also always be recomputed if needed.
 
 pub struct SymbolMap<'tcx> {
     index: FnvHashMap<TransItem<'tcx>, (usize, usize)>,
@@ -112,4 +114,15 @@ impl<'tcx> SymbolMap<'tcx> {
             &self.arena[start_index .. end_index]
         })
     }
+
+    pub fn get_or_compute<'map, 'scx>(&'map self,
+                                      scx: &SharedCrateContext<'scx, 'tcx>,
+                                      trans_item: TransItem<'tcx>)
+                                      -> Cow<'map, str> {
+        if let Some(sym) = self.get(trans_item) {
+            Cow::from(sym)
+        } else {
+            Cow::from(trans_item.compute_symbol_name(scx))
+        }
+    }
 }

src/librustc_trans/trans_item.rs

@@ -222,7 +222,7 @@ impl<'a, 'tcx> TransItem<'tcx> {
 
         assert!(declare::get_defined_value(ccx, symbol_name).is_none());
         let llfn = declare::declare_cfn(ccx, symbol_name, llfnty);
-	llvm::SetLinkage(llfn, linkage);
+        llvm::SetLinkage(llfn, linkage);
         attributes::set_frame_pointer_elimination(ccx, llfn);
         ccx.drop_glues().borrow_mut().insert(dg, (llfn, fn_ty));
     }