Update a few comments about symbol visibility.

Author: michaelwoerister

src/librustc_mir/monomorphize/collector.rs

@@ -762,14 +762,22 @@ fn should_monomorphize_locally<'a, 'tcx>(tcx: TyCtxt<'a, 'tcx, 'tcx>, instance:
                                                -> bool {
         debug_assert!(!def_id.is_local());
 
+        // If we are not in share generics mode, we don't link to upstream
+        // monomorphizations but always instantiate our own internal versions
+        // instead.
         if !tcx.share_generics() {
             return false
         }
 
+        // If this instance has no type parameters, it cannot be a shared
+        // monomorphization. Non-generic instances are already handled above
+        // by `is_reachable_non_generic()`
         if substs.types().next().is_none() {
             return false
         }
 
+        // Take a look at the available monomorphizations listed in the metadata
+        // of upstream crates.
         tcx.upstream_monomorphizations_for(def_id)
            .map(|set| set.contains_key(substs))
            .unwrap_or(false)

src/librustc_mir/monomorphize/partitioning.rs

@@ -301,6 +301,11 @@ fn place_root_translation_items<'a, 'tcx, I>(tcx: TyCtxt<'a, 'tcx, 'tcx>,
     let mut codegen_units = FxHashMap();
     let is_incremental_build = tcx.sess.opts.incremental.is_some();
     let mut internalization_candidates = FxHashSet();
+
+    // Determine if monomorphizations instantiated in this crate will be made
+    // available to downstream crates. This depends on whether we are in
+    // share-generics mode and whether the current crate can even have
+    // downstream crates.
     let export_generics = tcx.share_generics() &&
                           tcx.local_crate_exports_generics();
 

src/librustc_trans/callee.rs

@@ -118,59 +118,57 @@ pub fn get_fn<'a, 'tcx>(cx: &CodegenCx<'a, 'tcx>,
         // This is sort of subtle. Inside our codegen unit we started off
         // compilation by predefining all our own `TransItem` instances. That
         // is, everything we're translating ourselves is already defined. That
-        // means that anything we're actually translating ourselves will have
-        // hit the above branch in `get_declared_value`. As a result, we're
-        // guaranteed here that we're declaring a symbol that won't get defined,
-        // or in other words we're referencing a foreign value.
+        // means that anything we're actually translating in this codegen unit
+        // will have hit the above branch in `get_declared_value`. As a result,
+        // we're guaranteed here that we're declaring a symbol that won't get
+        // defined, or in other words we're referencing a value from another
+        // codegen unit or even another crate.
         //
         // So because this is a foreign value we blanket apply an external
         // linkage directive because it's coming from a different object file.
         // The visibility here is where it gets tricky. This symbol could be
         // referencing some foreign crate or foreign library (an `extern`
         // block) in which case we want to leave the default visibility. We may
-        // also, though, have multiple codegen units.
-        //
-        // In the situation of multiple codegen units this function may be
-        // referencing a function from another codegen unit. If we're
-        // indeed referencing a symbol in another codegen unit then we're in one
-        // of two cases:
-        //
-        //  * This is a symbol defined in a foreign crate and we're just
-        //    monomorphizing in another codegen unit. In this case this symbols
-        //    is for sure not exported, so both codegen units will be using
-        //    hidden visibility. Hence, we apply a hidden visibility here.
-        //
-        //  * This is a symbol defined in our local crate. If the symbol in the
-        //    other codegen unit is also not exported then like with the foreign
-        //    case we apply a hidden visibility. If the symbol is exported from
-        //    the foreign object file, however, then we leave this at the
-        //    default visibility as we'll just import it naturally.
+        // also, though, have multiple codegen units. It could be a
+        // monomorphization, in which case its expected visibility depends on
+        // whether we are sharing generics or not. The important thing here is
+        // that the visibility we apply to the declaration is the same one that
+        // has been applied to the definition (wherever that definition may be).
         unsafe {
             llvm::LLVMRustSetLinkage(llfn, llvm::Linkage::ExternalLinkage);
 
             let is_generic = instance.substs.types().next().is_some();
 
             if is_generic {
+                // This is a monomorphization. Its expected visibility depends
+                // on whether we are in share-generics mode.
+
                 if cx.tcx.share_generics() {
                     // We are in share_generics mode.
 
                     if instance_def_id.is_local() {
                         // This is a definition from the current crate. If the
                         // definition is unreachable for downstream crates or
                         // the current crate does not re-export generics, the
-                        // instance has been hidden
+                        // definition of the instance will have been declared
+                        // as `hidden`.
                         if cx.tcx.is_unreachable_local_definition(instance_def_id) ||
                            !cx.tcx.local_crate_exports_generics() {
                             llvm::LLVMRustSetVisibility(llfn, llvm::Visibility::Hidden);
                         }
                     } else {
+                        // This is a monomorphization of a generic function
+                        // defined in an upstream crate.
                         if cx.tcx.upstream_monomorphizations_for(instance_def_id)
                                  .map(|set| set.contains_key(instance.substs))
                                  .unwrap_or(false) {
-                            // This is instantiated in another crate. It cannot be hidden
+                            // This is instantiated in another crate. It cannot
+                            // be `hidden`.
                         } else {
                             // This is a local instantiation of an upstream definition.
-                            // If the current crate does not re-export it, it is hidden.
+                            // If the current crate does not re-export it
+                            // (because it is a C library or an executable), it
+                            // will have been declared `hidden`.
                             if !cx.tcx.local_crate_exports_generics() {
                                 llvm::LLVMRustSetVisibility(llfn, llvm::Visibility::Hidden);
                             }