Rework the visibility computation algorithm in preparation

for distinguishing type vs. value visibility.

The changes to the visibility of explicit specializations
are intentional.  The change to the "ugly" test case is
a consequence of a sensible implementation, and I am happy
to argue that this is better behavior.  Other changes may
or may not be intended;  it is quite difficult to divine
intent from some of the code I altered.

I've left behind a comment which I hope explains the
philosophy behind visibility computation.

git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@175326 91177308-0d34-0410-b5e6-96231b3b80d8

include/clang/AST/Decl.h

@@ -247,63 +247,45 @@ public:
     bool visibilityExplicit() const { return explicit_; }
 
     void setLinkage(Linkage L) { linkage_ = L; }
+
     void mergeLinkage(Linkage L) {
       setLinkage(minLinkage(linkage(), L));
     }
-    void mergeLinkage(LinkageInfo Other) {
-      mergeLinkage(Other.linkage());
+    void mergeLinkage(LinkageInfo other) {
+      mergeLinkage(other.linkage());
     }
 
-    // Merge the visibility V giving preference to explicit ones.
-    // This is used, for example, when merging the visibility of a class
-    // down to one of its members. If the member has no explicit visibility,
-    // the class visibility wins.
-    void mergeVisibility(Visibility V, bool E = false) {
-      // Never increase the visibility
-      if (visibility() < V)
+    /// Merge in the visibility 'newVis'.
+    void mergeVisibility(Visibility newVis, bool newExplicit) {
+      Visibility oldVis = visibility();
+
+      // Never increase visibility.
+      if (oldVis < newVis)
         return;
 
-      // If we have an explicit visibility, keep it
-      if (visibilityExplicit())
+      // If the new visibility is the same as the old and the new
+      // visibility isn't explicit, we have nothing to add.
+      if (oldVis == newVis && !newExplicit)
         return;
 
-      setVisibility(V, E);
+      // Otherwise, we're either decreasing visibility or making our
+      // existing visibility explicit.
+      setVisibility(newVis, newExplicit);
     }
-    // Merge the visibility V, keeping the most restrictive one.
-    // This is used for cases like merging the visibility of a template
-    // argument to an instantiation. If we already have a hidden class,
-    // no argument should give it default visibility.
-    void mergeVisibilityWithMin(Visibility V, bool E = false) {
-      // Never increase the visibility
-      if (visibility() < V)
-        return;
-
-      // FIXME: this
-      // If this visibility is explicit, keep it.
-      if (visibilityExplicit() && !E)
-        return;
-
-      // should be replaced with this
-      // Don't lose the explicit bit for nothing
-      //      if (visibility() == V && visibilityExplicit())
-      //        return;
-
-      setVisibility(V, E);
-    }
-    void mergeVisibility(LinkageInfo Other) {
-      mergeVisibility(Other.visibility(), Other.visibilityExplicit());
-    }
-    void mergeVisibilityWithMin(LinkageInfo Other) {
-      mergeVisibilityWithMin(Other.visibility(), Other.visibilityExplicit());
+    void mergeVisibility(LinkageInfo other) {
+      mergeVisibility(other.visibility(), other.visibilityExplicit());
     }
 
-    void merge(LinkageInfo Other) {
-      mergeLinkage(Other);
-      mergeVisibility(Other);
+    /// Merge both linkage and visibility.
+    void merge(LinkageInfo other) {
+      mergeLinkage(other);
+      mergeVisibility(other);
     }
-    void mergeWithMin(LinkageInfo Other) {
-      mergeLinkage(Other);
-      mergeVisibilityWithMin(Other);
+
+    /// Merge linkage and conditionally merge visibility.
+    void mergeMaybeWithVisibility(LinkageInfo other, bool withVis) {
+      mergeLinkage(other);
+      if (withVis) mergeVisibility(other);
     }
   };
 

include/clang/AST/DeclTemplate.h

@@ -84,6 +84,13 @@ public:
 
   unsigned size() const { return NumParams; }
 
+  llvm::ArrayRef<NamedDecl*> asArray() {
+    return llvm::ArrayRef<NamedDecl*>(begin(), size());
+  }
+  llvm::ArrayRef<const NamedDecl*> asArray() const {
+    return llvm::ArrayRef<const NamedDecl*>(begin(), size());
+  }
+
   NamedDecl* getParam(unsigned Idx) {
     assert(Idx < size() && "Template parameter index out-of-range");
     return begin()[Idx];
@@ -193,6 +200,11 @@ public:
   /// \brief Retrieve the template argument at a given index.
   const TemplateArgument &operator[](unsigned Idx) const { return get(Idx); }
 
+  /// \brief Produce this as an array ref.
+  llvm::ArrayRef<TemplateArgument> asArray() const {
+    return llvm::ArrayRef<TemplateArgument>(data(), size());
+  }
+
   /// \brief Retrieve the number of template arguments in this
   /// template argument list.
   unsigned size() const { return NumArguments; }
@@ -324,6 +336,23 @@ public:
     return getTemplateSpecializationKind() == TSK_ExplicitSpecialization;
   }
 
+  /// \brief True if this declaration is an explicit specialization,
+  /// explicit instantiation declaration, or explicit instantiation
+  /// definition.
+  bool isExplicitInstantiationOrSpecialization() const {
+    switch (getTemplateSpecializationKind()) {
+    case TSK_ExplicitSpecialization:
+    case TSK_ExplicitInstantiationDeclaration:
+    case TSK_ExplicitInstantiationDefinition:
+      return true;
+
+    case TSK_Undeclared:
+    case TSK_ImplicitInstantiation:
+      return false;
+    }
+    llvm_unreachable("bad template specialization kind");
+  }
+
   /// \brief Set the template specialization kind.
   void setTemplateSpecializationKind(TemplateSpecializationKind TSK) {
     assert(TSK != TSK_Undeclared &&
@@ -1433,6 +1462,23 @@ public:
     return getSpecializationKind() == TSK_ExplicitSpecialization;
   }
 
+  /// \brief True if this declaration is an explicit specialization,
+  /// explicit instantiation declaration, or explicit instantiation
+  /// definition.
+  bool isExplicitInstantiationOrSpecialization() const {
+    switch (getTemplateSpecializationKind()) {
+    case TSK_ExplicitSpecialization:
+    case TSK_ExplicitInstantiationDeclaration:
+    case TSK_ExplicitInstantiationDefinition:
+      return true;
+
+    case TSK_Undeclared:
+    case TSK_ImplicitInstantiation:
+      return false;
+    }
+    llvm_unreachable("bad template specialization kind");
+  }
+
   void setSpecializationKind(TemplateSpecializationKind TSK) {
     SpecializationKind = TSK;
   }

include/clang/AST/TemplateBase.h

@@ -317,6 +317,12 @@ public:
     return Args.NumArgs;
   }
 
+  /// \brief Return the array of arguments in this template argument pack.
+  llvm::ArrayRef<TemplateArgument> getPackAsArray() const {
+    assert(Kind == Pack);
+    return llvm::ArrayRef<TemplateArgument>(Args.Args, Args.NumArgs);
+  }
+
   /// \brief Determines whether two template arguments are superficially the
   /// same.
   bool structurallyEquals(const TemplateArgument &Other) const;

lib/AST/Decl.cpp

@@ -37,6 +37,53 @@ using namespace clang;
 // NamedDecl Implementation
 //===----------------------------------------------------------------------===//
 
+// Visibility rules aren't rigorously externally specified, but here
+// are the basic principles behind what we implement:
+//
+// 1. An explicit visibility attribute is generally a direct expression
+// of the user's intent and should be honored.  Only the innermost
+// visibility attribute applies.  If no visibility attribute applies,
+// global visibility settings are considered.
+//
+// 2. There is one caveat to the above: on or in a template pattern,
+// an explicit visibility attribute is just a default rule, and
+// visibility can be decreased by the visibility of template
+// arguments.  But this, too, has an exception: an attribute on an
+// explicit specialization or instantiation causes all the visibility
+// restrictions of the template arguments to be ignored.
+//
+// 3. A variable that does not otherwise have explicit visibility can
+// be restricted by the visibility of its type.
+//
+// 4. A visibility restriction is explicit if it comes from an
+// attribute (or something like it), not a global visibility setting.
+// When emitting a reference to an external symbol, visibility
+// restrictions are ignored unless they are explicit.
+
+/// Kinds of LV computation.  The linkage side of the computation is
+/// always the same, but different things can change how visibility is
+/// computed.
+enum LVComputationKind {
+  /// Do an LV computation that does everything normal for linkage but
+  /// ignores sources of visibility other than template arguments.
+  LVOnlyTemplateArguments,
+
+  /// Do a normal LV computation for, ultimately, a type.
+  LVForType,
+
+  /// Do a normal LV computation for, ultimately, a value.
+  LVForValue
+};
+
+typedef NamedDecl::LinkageInfo LinkageInfo;
+
+/// Is the given declaration a "type" or a "value" for the purposes of
+/// visibility computation?
+static bool usesTypeVisibility(const NamedDecl *D) {
+  return isa<TypeDecl>(D) || isa<ClassTemplateDecl>(D);
+}
+
+/// Return the explicit visibility of the given declaration.
 static llvm::Optional<Visibility> getVisibilityOf(const Decl *D) {
   // If this declaration has an explicit visibility attribute, use it.
   if (const VisibilityAttr *A = D->getAttr<VisibilityAttr>()) {
@@ -64,40 +111,63 @@ static llvm::Optional<Visibility> getVisibilityOf(const Decl *D) {
   return llvm::Optional<Visibility>();
 }
 
-typedef NamedDecl::LinkageInfo LinkageInfo;
-
 static LinkageInfo getLVForType(QualType T) {
   std::pair<Linkage,Visibility> P = T->getLinkageAndVisibility();
   return LinkageInfo(P.first, P.second, T->isVisibilityExplicit());
 }
 
 /// \brief Get the most restrictive linkage for the types in the given
-/// template parameter list.
+/// template parameter list.  For visibility purposes, template
+/// parameters are part of the signature of a template.
 static LinkageInfo
-getLVForTemplateParameterList(const TemplateParameterList *Params) {
-  LinkageInfo LV(ExternalLinkage, DefaultVisibility, false);
-  for (TemplateParameterList::const_iterator P = Params->begin(),
-                                          PEnd = Params->end();
+getLVForTemplateParameterList(const TemplateParameterList *params) {
+  LinkageInfo LV;
+  for (TemplateParameterList::const_iterator P = params->begin(),
+                                          PEnd = params->end();
        P != PEnd; ++P) {
+
+    // Template type parameters are the most common and never
+    // contribute to visibility, pack or not.
+    if (isa<TemplateTypeParmDecl>(*P))
+      continue;
+
+    // Non-type template parameters can be restricted by the value type, e.g.
+    //   template <enum X> class A { ... };
+    // We have to be careful here, though, because we can be dealing with
+    // dependent types.
     if (NonTypeTemplateParmDecl *NTTP = dyn_cast<NonTypeTemplateParmDecl>(*P)) {
-      if (NTTP->isExpandedParameterPack()) {
-        for (unsigned I = 0, N = NTTP->getNumExpansionTypes(); I != N; ++I) {
-          QualType T = NTTP->getExpansionType(I);
-          if (!T->isDependentType())
-            LV.merge(getLVForType(T));
+      // Handle the non-pack case first.
+      if (!NTTP->isExpandedParameterPack()) {
+        if (!NTTP->getType()->isDependentType()) {
+          LV.merge(getLVForType(NTTP->getType()));
         }
         continue;
       }
 
-      if (!NTTP->getType()->isDependentType()) {
-        LV.merge(getLVForType(NTTP->getType()));
-        continue;
+      // Look at all the types in an expanded pack.
+      for (unsigned i = 0, n = NTTP->getNumExpansionTypes(); i != n; ++i) {
+        QualType type = NTTP->getExpansionType(i);
+        if (!type->isDependentType())
+          LV.merge(getLVForType(type));
       }
+      continue;
     }
 
-    if (TemplateTemplateParmDecl *TTP
-                                   = dyn_cast<TemplateTemplateParmDecl>(*P)) {
+    // Template template parameters can be restricted by their
+    // template parameters, recursively.
+    TemplateTemplateParmDecl *TTP = cast<TemplateTemplateParmDecl>(*P);
+
+    // Handle the non-pack case first.
+    if (!TTP->isExpandedParameterPack()) {
       LV.merge(getLVForTemplateParameterList(TTP->getTemplateParameters()));
+      continue;
+    }
+
+    // Look at all expansions in an expanded pack.
+    for (unsigned i = 0, n = TTP->getNumExpansionTemplateParameters();
+           i != n; ++i) {
+      LV.merge(getLVForTemplateParameterList(
+                                    TTP->getExpansionTemplateParameters(i)));
     }
   }
 
@@ -105,67 +175,137 @@ getLVForTemplateParameterList(const TemplateParameterList *Params) {
 }
 
 /// getLVForDecl - Get the linkage and visibility for the given declaration.
-static LinkageInfo getLVForDecl(const NamedDecl *D, bool OnlyTemplate);
+static LinkageInfo getLVForDecl(const NamedDecl *D,
+                                LVComputationKind computation);
 
 /// \brief Get the most restrictive linkage for the types and
 /// declarations in the given template argument list.
-static LinkageInfo getLVForTemplateArgumentList(const TemplateArgument *Args,
-                                                unsigned NumArgs,
-                                                bool OnlyTemplate) {
-  LinkageInfo LV(ExternalLinkage, DefaultVisibility, false);
+///
+/// Note that we don't take an LVComputationKind because we always
+/// want to honor the visibility of template arguments in the same way.
+static LinkageInfo
+getLVForTemplateArgumentList(ArrayRef<TemplateArgument> args) {
+  LinkageInfo LV;
 
-  for (unsigned I = 0; I != NumArgs; ++I) {
-    switch (Args[I].getKind()) {
+  for (unsigned i = 0, e = args.size(); i != e; ++i) {
+    const TemplateArgument &arg = args[i];
+    switch (arg.getKind()) {
     case TemplateArgument::Null:
     case TemplateArgument::Integral:
     case TemplateArgument::Expression:
-      break;
+      continue;
 
     case TemplateArgument::Type:
-      LV.mergeWithMin(getLVForType(Args[I].getAsType()));
-      break;
+      LV.merge(getLVForType(arg.getAsType()));
+      continue;
 
     case TemplateArgument::Declaration:
-      if (NamedDecl *ND = dyn_cast<NamedDecl>(Args[I].getAsDecl()))
-        LV.mergeWithMin(getLVForDecl(ND, OnlyTemplate));
-      break;
+      if (NamedDecl *ND = dyn_cast<NamedDecl>(arg.getAsDecl())) {
+        assert(!usesTypeVisibility(ND));
+        LV.merge(getLVForDecl(ND, LVForValue));
+      }
+      continue;
 
     case TemplateArgument::NullPtr:
-      LV.mergeWithMin(getLVForType(Args[I].getNullPtrType()));
-      break;
+      LV.merge(getLVForType(arg.getNullPtrType()));
+      continue;
 
     case TemplateArgument::Template:
     case TemplateArgument::TemplateExpansion:
       if (TemplateDecl *Template
-                = Args[I].getAsTemplateOrTemplatePattern().getAsTemplateDecl())
-        LV.mergeWithMin(getLVForDecl(Template, OnlyTemplate));
-      break;
+                = arg.getAsTemplateOrTemplatePattern().getAsTemplateDecl())
+        LV.merge(getLVForDecl(Template, LVForValue));
+      continue;
 
     case TemplateArgument::Pack:
-      LV.mergeWithMin(getLVForTemplateArgumentList(Args[I].pack_begin(),
-                                                   Args[I].pack_size(),
-                                                   OnlyTemplate));
-      break;
+      LV.merge(getLVForTemplateArgumentList(arg.getPackAsArray()));
+      continue;
     }
+    llvm_unreachable("bad template argument kind");
   }
 
   return LV;
 }
 
 static LinkageInfo
-getLVForTemplateArgumentList(const TemplateArgumentList &TArgs,
-                             bool OnlyTemplate) {
-  return getLVForTemplateArgumentList(TArgs.data(), TArgs.size(), OnlyTemplate);
+getLVForTemplateArgumentList(const TemplateArgumentList &TArgs) {
+  return getLVForTemplateArgumentList(TArgs.asArray());
 }
 
-static bool shouldConsiderTemplateVis(const FunctionDecl *fn,
-                               const FunctionTemplateSpecializationInfo *spec) {
-  return !fn->hasAttr<VisibilityAttr>() || spec->isExplicitSpecialization();
+/// Merge in template-related linkage and visibility for the given
+/// function template specialization.
+///
+/// We don't need a computation kind here because we can assume
+/// LVForValue.
+static void mergeTemplateLV(LinkageInfo &LV, const FunctionDecl *fn,
+                      const FunctionTemplateSpecializationInfo *specInfo) {
+  bool hasExplicitVisibility = fn->hasAttr<VisibilityAttr>();
+  FunctionTemplateDecl *temp = specInfo->getTemplate();
+
+  // Include visibility from the template parameters and arguments
+  // only if this is not an explicit instantiation or specialization
+  // with direct explicit visibility.  (Implicit instantiations won't
+  // have a direct attribute.)
+  bool considerVisibility = !hasExplicitVisibility;
+
+  // Merge information from the template parameters.
+  LinkageInfo tempLV =
+    getLVForTemplateParameterList(temp->getTemplateParameters());
+  LV.mergeMaybeWithVisibility(tempLV, considerVisibility);
+
+  // Merge information from the template arguments.
+  const TemplateArgumentList &templateArgs = *specInfo->TemplateArguments;
+  LinkageInfo argsLV = getLVForTemplateArgumentList(templateArgs);
+  LV.mergeMaybeWithVisibility(argsLV, considerVisibility);
 }
 
-static bool
-shouldConsiderTemplateVis(const ClassTemplateSpecializationDecl *d) {
-  return !d->hasAttr<VisibilityAttr>() || d->isExplicitSpecialization();
+/// Merge in template-related linkage and visibility for the given
+/// class template specialization.
+static void mergeTemplateLV(LinkageInfo &LV,
+                            const ClassTemplateSpecializationDecl *spec,
+                            LVComputationKind computation) {
+  // FIXME: type visibility
+  bool hasExplicitVisibility = spec->hasAttr<VisibilityAttr>();
+  ClassTemplateDecl *temp = spec->getSpecializedTemplate();
+
+  // Include visibility from the template parameters and arguments
+  // only if this is not an explicit instantiation or specialization
+  // with direct explicit visibility (and note that implicit
+  // instantiations won't have a direct attribute).
+  //
+  // Furthermore, we want to ignore template parameters and arguments
+  // for an explicit instantiation or specialization when computing
+  // the visibility of a member thereof with explicit visibility.
+  //
+  // This is a bit complex; let's unpack it.
+  //
+  // An explicit class specialization is an independent, top-level
+  // declaration.  As such, if it or any of its members has an
+  // explicit visibility attribute, that must directly express the
+  // user's intent, and we should honor it.  The same logic applies to
+  // an explicit instantiation of a member of such a thing.
+  //
+  // That we're doing this for a member with explicit visibility
+  // is encoded by the computation kind being OnlyTemplateArguments.
+  bool considerVisibility =
+   !(hasExplicitVisibility ||
+     (computation == LVOnlyTemplateArguments &&
+      spec->isExplicitInstantiationOrSpecialization()));
+
+  // Merge information from the template parameters, but ignore
+  // visibility if we're only considering template arguments.
+
+  LinkageInfo tempLV =
+    getLVForTemplateParameterList(temp->getTemplateParameters());
+  LV.mergeMaybeWithVisibility(tempLV,
+               considerVisibility && computation != LVOnlyTemplateArguments);
+
+  // Merge information from the template arguments.  We ignore
+  // template-argument visibility if we've got an explicit
+  // instantiation with a visibility attribute.
+  const TemplateArgumentList &templateArgs = spec->getTemplateArgs();
+  LinkageInfo argsLV = getLVForTemplateArgumentList(templateArgs);
+  LV.mergeMaybeWithVisibility(argsLV, considerVisibility);
 }
 
 static bool useInlineVisibilityHidden(const NamedDecl *D) {
@@ -202,7 +342,7 @@ template <typename T> static bool isInExternCContext(T *D) {
 }
 
 static LinkageInfo getLVForNamespaceScopeDecl(const NamedDecl *D,
-                                              bool OnlyTemplate) {
+                                              LVComputationKind computation) {
   assert(D->getDeclContext()->getRedeclContext()->isFileContext() &&
          "Not a name having namespace scope");
   ASTContext &Context = D->getASTContext();
@@ -280,12 +420,12 @@ static LinkageInfo getLVForNamespaceScopeDecl(const NamedDecl *D,
   //   external.
   LinkageInfo LV;
 
-  if (!OnlyTemplate) {
+  if (computation != LVOnlyTemplateArguments) {
     if (llvm::Optional<Visibility> Vis = D->getExplicitVisibility()) {
       LV.mergeVisibility(*Vis, true);
     } else {
       // If we're declared in a namespace with a visibility attribute,
-      // use that namespace's visibility, but don't call it explicit.
+      // use that namespace's visibility, and it still counts as explicit.
       for (const DeclContext *DC = D->getDeclContext();
            !isa<TranslationUnitDecl>(DC);
            DC = DC->getParent()) {
@@ -297,14 +437,19 @@ static LinkageInfo getLVForNamespaceScopeDecl(const NamedDecl *D,
         }
       }
     }
-  }
 
-  if (!OnlyTemplate) {
-    LV.mergeVisibility(Context.getLangOpts().getVisibilityMode());
-    // If we're paying attention to global visibility, apply
-    // -finline-visibility-hidden if this is an inline method.
-    if (!LV.visibilityExplicit() && useInlineVisibilityHidden(D))
-      LV.mergeVisibility(HiddenVisibility, true);
+    // Add in global settings if the above didn't give us direct visibility.
+    if (!LV.visibilityExplicit()) {
+      // FIXME: type visibility
+      LV.mergeVisibility(Context.getLangOpts().getVisibilityMode(),
+                         /*explicit*/ false);
+
+
+      // If we're paying attention to global visibility, apply
+      // -finline-visibility-hidden if this is an inline method.
+      if (useInlineVisibilityHidden(D))
+        LV.mergeVisibility(HiddenVisibility, true);
+    }
   }
 
   // C++ [basic.link]p4:
@@ -340,7 +485,8 @@ static LinkageInfo getLVForNamespaceScopeDecl(const NamedDecl *D,
       LinkageInfo TypeLV = getLVForType(Var->getType());
       if (TypeLV.linkage() != ExternalLinkage)
         return LinkageInfo::uniqueExternal();
-      LV.mergeVisibility(TypeLV);
+      if (!LV.visibilityExplicit())
+        LV.mergeVisibility(TypeLV);
     }
 
     if (Var->getStorageClass() == SC_PrivateExtern)
@@ -377,17 +523,7 @@ static LinkageInfo getLVForNamespaceScopeDecl(const NamedDecl *D,
     // this is an explicit specialization with a visibility attribute.
     if (FunctionTemplateSpecializationInfo *specInfo
                                = Function->getTemplateSpecializationInfo()) {
-      LinkageInfo TempLV = getLVForDecl(specInfo->getTemplate(), true);
-      const TemplateArgumentList &templateArgs = *specInfo->TemplateArguments;
-      LinkageInfo ArgsLV = getLVForTemplateArgumentList(templateArgs,
-                                                        OnlyTemplate);
-      if (shouldConsiderTemplateVis(Function, specInfo)) {
-        LV.mergeWithMin(TempLV);
-        LV.mergeWithMin(ArgsLV);
-      } else {
-        LV.mergeLinkage(TempLV);
-        LV.mergeLinkage(ArgsLV);
-      }
+      mergeTemplateLV(LV, Function, specInfo);
     }
 
   //     - a named class (Clause 9), or an unnamed class defined in a
@@ -405,26 +541,13 @@ static LinkageInfo getLVForNamespaceScopeDecl(const NamedDecl *D,
     // linkage of the template and template arguments.
     if (const ClassTemplateSpecializationDecl *spec
           = dyn_cast<ClassTemplateSpecializationDecl>(Tag)) {
-      // From the template.
-      LinkageInfo TempLV = getLVForDecl(spec->getSpecializedTemplate(), true);
-
-      // The arguments at which the template was instantiated.
-      const TemplateArgumentList &TemplateArgs = spec->getTemplateArgs();
-      LinkageInfo ArgsLV = getLVForTemplateArgumentList(TemplateArgs,
-                                                        OnlyTemplate);
-      if (shouldConsiderTemplateVis(spec)) {
-        LV.mergeWithMin(TempLV);
-        LV.mergeWithMin(ArgsLV);
-      } else {
-        LV.mergeLinkage(TempLV);
-        LV.mergeLinkage(ArgsLV);
-      }
+      mergeTemplateLV(LV, spec, computation);
     }
 
   //     - an enumerator belonging to an enumeration with external linkage;
   } else if (isa<EnumConstantDecl>(D)) {
     LinkageInfo EnumLV = getLVForDecl(cast<NamedDecl>(D->getDeclContext()),
-                                      OnlyTemplate);
+                                      computation);
     if (!isExternalLinkage(EnumLV.linkage()))
       return LinkageInfo::none();
     LV.merge(EnumLV);
@@ -432,7 +555,11 @@ static LinkageInfo getLVForNamespaceScopeDecl(const NamedDecl *D,
   //     - a template, unless it is a function template that has
   //       internal linkage (Clause 14);
   } else if (const TemplateDecl *temp = dyn_cast<TemplateDecl>(D)) {
-    LV.merge(getLVForTemplateParameterList(temp->getTemplateParameters()));
+    bool considerVisibility = (computation != LVOnlyTemplateArguments);
+    LinkageInfo tempLV =
+      getLVForTemplateParameterList(temp->getTemplateParameters());
+    LV.mergeMaybeWithVisibility(tempLV, considerVisibility);
+
   //     - a namespace (7.3), unless it is declared within an unnamed
   //       namespace.
   } else if (isa<NamespaceDecl>(D) && !D->isInAnonymousNamespace()) {
@@ -456,7 +583,8 @@ static LinkageInfo getLVForNamespaceScopeDecl(const NamedDecl *D,
   return LV;
 }
 
-static LinkageInfo getLVForClassMember(const NamedDecl *D, bool OnlyTemplate) {
+static LinkageInfo getLVForClassMember(const NamedDecl *D,
+                                       LVComputationKind computation) {
   // Only certain class members have linkage.  Note that fields don't
   // really have linkage, but it's convenient to say they do for the
   // purposes of calculating linkage of pointer-to-data-member
@@ -470,7 +598,7 @@ static LinkageInfo getLVForClassMember(const NamedDecl *D, bool OnlyTemplate) {
   LinkageInfo LV;
 
   // If we have an explicit visibility attribute, merge that in.
-  if (!OnlyTemplate) {
+  if (computation != LVOnlyTemplateArguments) {
     if (llvm::Optional<Visibility> Vis = D->getExplicitVisibility())
       LV.mergeVisibility(*Vis, true);
     // If we're paying attention to global visibility, apply
@@ -485,15 +613,16 @@ static LinkageInfo getLVForClassMember(const NamedDecl *D, bool OnlyTemplate) {
   // If this class member has an explicit visibility attribute, the only
   // thing that can change its visibility is the template arguments, so
   // only look for them when processing the class.
-  bool ClassOnlyTemplate =  LV.visibilityExplicit() ? true : OnlyTemplate;
+  LVComputationKind classComputation =
+    (LV.visibilityExplicit() ? LVOnlyTemplateArguments : computation);
 
   // If this member has an visibility attribute, ClassF will exclude
   // attributes on the class or command line options, keeping only information
   // about the template instantiation. If the member has no visibility
   // attributes, mergeWithMin behaves like merge, so in both cases mergeWithMin
   // produces the desired result.
-  LV.mergeWithMin(getLVForDecl(cast<RecordDecl>(D->getDeclContext()),
-                               ClassOnlyTemplate));
+  LV.merge(getLVForDecl(cast<RecordDecl>(D->getDeclContext()),
+                        classComputation));
   if (!isExternalLinkage(LV.linkage()))
     return LinkageInfo::none();
 
@@ -501,9 +630,6 @@ static LinkageInfo getLVForClassMember(const NamedDecl *D, bool OnlyTemplate) {
   if (LV.linkage() == UniqueExternalLinkage)
     return LinkageInfo::uniqueExternal();
 
-  if (!OnlyTemplate)
-    LV.mergeVisibility(D->getASTContext().getLangOpts().getVisibilityMode());
-
   if (const CXXMethodDecl *MD = dyn_cast<CXXMethodDecl>(D)) {
     // If the type of the function uses a type with unique-external
     // linkage, it's not legally usable from outside this translation unit.
@@ -514,21 +640,7 @@ static LinkageInfo getLVForClassMember(const NamedDecl *D, bool OnlyTemplate) {
     // the template parameters and arguments.
     if (FunctionTemplateSpecializationInfo *spec
            = MD->getTemplateSpecializationInfo()) {
-      const TemplateArgumentList &TemplateArgs = *spec->TemplateArguments;
-      LinkageInfo ArgsLV = getLVForTemplateArgumentList(TemplateArgs,
-                                                        OnlyTemplate);
-      TemplateParameterList *TemplateParams =
-        spec->getTemplate()->getTemplateParameters();
-      LinkageInfo ParamsLV = getLVForTemplateParameterList(TemplateParams);
-      if (shouldConsiderTemplateVis(MD, spec)) {
-        LV.mergeWithMin(ArgsLV);
-        if (!OnlyTemplate)
-          LV.mergeWithMin(ParamsLV);
-      } else {
-        LV.mergeLinkage(ArgsLV);
-        if (!OnlyTemplate)
-          LV.mergeLinkage(ParamsLV);
-      }
+      mergeTemplateLV(LV, MD, spec);
     }
 
     // Note that in contrast to basically every other situation, we
@@ -537,33 +649,26 @@ static LinkageInfo getLVForClassMember(const NamedDecl *D, bool OnlyTemplate) {
   } else if (const CXXRecordDecl *RD = dyn_cast<CXXRecordDecl>(D)) {
     if (const ClassTemplateSpecializationDecl *spec
         = dyn_cast<ClassTemplateSpecializationDecl>(RD)) {
-      // Merge template argument/parameter information for member
-      // class template specializations.
-      const TemplateArgumentList &TemplateArgs = spec->getTemplateArgs();
-      LinkageInfo ArgsLV = getLVForTemplateArgumentList(TemplateArgs,
-                                                        OnlyTemplate);
-      TemplateParameterList *TemplateParams =
-        spec->getSpecializedTemplate()->getTemplateParameters();
-      LinkageInfo ParamsLV = getLVForTemplateParameterList(TemplateParams);
-      if (shouldConsiderTemplateVis(spec)) {
-        LV.mergeWithMin(ArgsLV);
-        if (!OnlyTemplate)
-          LV.mergeWithMin(ParamsLV);
-      } else {
-        LV.mergeLinkage(ArgsLV);
-        if (!OnlyTemplate)
-          LV.mergeLinkage(ParamsLV);
-      }
+      mergeTemplateLV(LV, spec, computation);
     }
 
   // Static data members.
   } else if (const VarDecl *VD = dyn_cast<VarDecl>(D)) {
     // Modify the variable's linkage by its type, but ignore the
     // type's visibility unless it's a definition.
-    LinkageInfo TypeLV = getLVForType(VD->getType());
-    if (TypeLV.linkage() != ExternalLinkage)
+    LinkageInfo typeLV = getLVForType(VD->getType());
+    if (typeLV.linkage() != ExternalLinkage)
       LV.mergeLinkage(UniqueExternalLinkage);
-    LV.mergeVisibility(TypeLV);
+    if (!LV.visibilityExplicit())
+      LV.mergeVisibility(typeLV);
+
+  // Template members.
+  } else if (const TemplateDecl *temp = dyn_cast<TemplateDecl>(D)) {
+    bool considerVisibility =
+      (!LV.visibilityExplicit() && computation != LVOnlyTemplateArguments);
+    LinkageInfo tempLV =
+      getLVForTemplateParameterList(temp->getTemplateParameters());
+    LV.mergeMaybeWithVisibility(tempLV, considerVisibility);
   }
 
   return LV;
@@ -618,7 +723,10 @@ Linkage NamedDecl::getLinkage() const {
   if (HasCachedLinkage)
     return Linkage(CachedLinkage);
 
-  CachedLinkage = getLVForDecl(this, true).linkage();
+  // We don't care about visibility here, so suppress all the
+  // unnecessary explicit-visibility checks by asking for a
+  // template-argument-only analysis.
+  CachedLinkage = getLVForDecl(this, LVOnlyTemplateArguments).linkage();
   HasCachedLinkage = 1;
 
 #ifndef NDEBUG
@@ -629,7 +737,9 @@ Linkage NamedDecl::getLinkage() const {
 }
 
 LinkageInfo NamedDecl::getLinkageAndVisibility() const {
-  LinkageInfo LI = getLVForDecl(this, false);
+  LVComputationKind computation =
+    (usesTypeVisibility(this) ? LVForType : LVForValue);
+  LinkageInfo LI = getLVForDecl(this, computation);
   if (HasCachedLinkage) {
     assert(Linkage(CachedLinkage) == LI.linkage());
     return LI;
@@ -729,7 +839,61 @@ llvm::Optional<Visibility> NamedDecl::getExplicitVisibility() const {
   return llvm::Optional<Visibility>();
 }
 
-static LinkageInfo getLVForDecl(const NamedDecl *D, bool OnlyTemplate) {
+static LinkageInfo getLVForLocalDecl(const NamedDecl *D,
+                                     LVComputationKind computation) {
+  if (const FunctionDecl *Function = dyn_cast<FunctionDecl>(D)) {
+    if (Function->isInAnonymousNamespace() &&
+        !Function->getDeclContext()->isExternCContext())
+      return LinkageInfo::uniqueExternal();
+
+    // This is a "void f();" which got merged with a file static.
+    if (Function->getStorageClass() == SC_Static)
+      return LinkageInfo::internal();
+
+    LinkageInfo LV;
+    if (computation != LVOnlyTemplateArguments) {
+      if (llvm::Optional<Visibility> Vis = Function->getExplicitVisibility())
+        LV.mergeVisibility(*Vis, true);
+    }
+
+    // Note that Sema::MergeCompatibleFunctionDecls already takes care of
+    // merging storage classes and visibility attributes, so we don't have to
+    // look at previous decls in here.
+
+    return LV;
+  }
+
+  if (const VarDecl *Var = dyn_cast<VarDecl>(D)) {
+    if (Var->getStorageClassAsWritten() == SC_Extern ||
+        Var->getStorageClassAsWritten() == SC_PrivateExtern) {
+      if (Var->isInAnonymousNamespace() &&
+          !Var->getDeclContext()->isExternCContext())
+        return LinkageInfo::uniqueExternal();
+
+      // This is an "extern int foo;" which got merged with a file static.
+      if (Var->getStorageClass() == SC_Static)
+        return LinkageInfo::internal();
+
+      LinkageInfo LV;
+      if (Var->getStorageClass() == SC_PrivateExtern)
+        LV.mergeVisibility(HiddenVisibility, true);
+      else if (computation != LVOnlyTemplateArguments) {
+        if (llvm::Optional<Visibility> Vis = Var->getExplicitVisibility())
+          LV.mergeVisibility(*Vis, true);
+      }
+
+      // Note that Sema::MergeVarDecl already takes care of implementing
+      // C99 6.2.2p4 and propagating the visibility attribute, so we don't
+      // have to do it here.
+      return LV;
+    }
+  }
+
+  return LinkageInfo::none();
+}
+
+static LinkageInfo getLVForDecl(const NamedDecl *D,
+                                LVComputationKind computation) {
   // Objective-C: treat all Objective-C declarations as having external
   // linkage.
   switch (D->getKind()) {
@@ -764,12 +928,11 @@ static LinkageInfo getLVForDecl(const NamedDecl *D, bool OnlyTemplate) {
           if (isa<ParmVarDecl>(ContextDecl))
             DC = ContextDecl->getDeclContext()->getRedeclContext();
           else
-            return getLVForDecl(cast<NamedDecl>(ContextDecl),
-                                OnlyTemplate);
+            return getLVForDecl(cast<NamedDecl>(ContextDecl), computation);
         }
 
         if (const NamedDecl *ND = dyn_cast<NamedDecl>(DC))
-          return getLVForDecl(ND, OnlyTemplate);
+          return getLVForDecl(ND, computation);
         
         return LinkageInfo::external();
       }
@@ -780,7 +943,7 @@ static LinkageInfo getLVForDecl(const NamedDecl *D, bool OnlyTemplate) {
 
   // Handle linkage for namespace-scope names.
   if (D->getDeclContext()->getRedeclContext()->isFileContext())
-    return getLVForNamespaceScopeDecl(D, OnlyTemplate);
+    return getLVForNamespaceScopeDecl(D, computation);
   
   // C++ [basic.link]p5:
   //   In addition, a member function, static data member, a named
@@ -790,7 +953,7 @@ static LinkageInfo getLVForDecl(const NamedDecl *D, bool OnlyTemplate) {
   //   purposes (7.1.3), has external linkage if the name of the class
   //   has external linkage.
   if (D->getDeclContext()->isRecord())
-    return getLVForClassMember(D, OnlyTemplate);
+    return getLVForClassMember(D, computation);
 
   // C++ [basic.link]p6:
   //   The name of a function declared in block scope and the name of
@@ -803,54 +966,8 @@ static LinkageInfo getLVForDecl(const NamedDecl *D, bool OnlyTemplate) {
   //   one such matching entity, the program is ill-formed. Otherwise,
   //   if no matching entity is found, the block scope entity receives
   //   external linkage.
-  if (D->getDeclContext()->isFunctionOrMethod()) {
-    if (const FunctionDecl *Function = dyn_cast<FunctionDecl>(D)) {
-      if (Function->isInAnonymousNamespace() &&
-          !Function->getDeclContext()->isExternCContext())
-        return LinkageInfo::uniqueExternal();
-
-      // This is a "void f();" which got merged with a file static.
-      if (Function->getStorageClass() == SC_Static)
-        return LinkageInfo::internal();
-
-      LinkageInfo LV;
-      if (!OnlyTemplate) {
-        if (llvm::Optional<Visibility> Vis = Function->getExplicitVisibility())
-          LV.mergeVisibility(*Vis, true);
-      }
-
-      // Note that Sema::MergeCompatibleFunctionDecls already takes care of
-      // merging storage classes and visibility attributes, so we don't have to
-      // look at previous decls in here.
-
-      return LV;
-    }
-
-    if (const VarDecl *Var = dyn_cast<VarDecl>(D))
-      if (Var->getStorageClassAsWritten() == SC_Extern ||
-          Var->getStorageClassAsWritten() == SC_PrivateExtern) {
-        if (Var->isInAnonymousNamespace() &&
-            !Var->getDeclContext()->isExternCContext())
-          return LinkageInfo::uniqueExternal();
-
-        // This is an "extern int foo;" which got merged with a file static.
-        if (Var->getStorageClass() == SC_Static)
-          return LinkageInfo::internal();
-
-        LinkageInfo LV;
-        if (Var->getStorageClass() == SC_PrivateExtern)
-          LV.mergeVisibility(HiddenVisibility, true);
-        else if (!OnlyTemplate) {
-          if (llvm::Optional<Visibility> Vis = Var->getExplicitVisibility())
-            LV.mergeVisibility(*Vis, true);
-        }
-
-        // Note that Sema::MergeVarDecl already takes care of implementing
-        // C99 6.2.2p4 and propagating the visibility attribute, so we don't
-        // have to do it here.
-        return LV;
-      }
-  }
+  if (D->getDeclContext()->isFunctionOrMethod())
+    return getLVForLocalDecl(D, computation);
 
   // C++ [basic.link]p6:
   //   Names not covered by these rules have no linkage.

test/CodeGenCXX/visibility.cpp

@@ -580,9 +580,7 @@ namespace PR10113 {
   };
   template class foo::bar<zed>;
   // CHECK: define weak_odr void @_ZN7PR101133foo3barINS_3zedEE3zedEv
-
-  // FIXME: This should be hidden as zed is hidden.
-  // CHECK-HIDDEN: define weak_odr void @_ZN7PR101133foo3barINS_3zedEE3zedEv
+  // CHECK-HIDDEN: define weak_odr hidden void @_ZN7PR101133foo3barINS_3zedEE3zedEv
 }
 
 namespace PR11690 {
@@ -613,9 +611,7 @@ namespace PR11690_2 {
   };
   template class foo::zed<baz>;
   // CHECK: define weak_odr void @_ZN9PR11690_23foo3zedINS_3bazENS0_3barEE3barEv
-
-  // FIXME: This should be hidden as baz is hidden.
-  // CHECK-HIDDEN: define weak_odr void @_ZN9PR11690_23foo3zedINS_3bazENS0_3barEE3barEv
+  // CHECK-HIDDEN: define weak_odr hidden void @_ZN9PR11690_23foo3zedINS_3bazENS0_3barEE3barEv
 }
 
 namespace test23 {
@@ -729,10 +725,10 @@ namespace test34 {
 
 namespace test35 {
   // This is a really ugly testcase. GCC propagates the DEFAULT in zed's
-  // definition. What we do instead is be conservative about merging
-  // implicit visibilities.
-  // FIXME: Maybe the best thing to do here is error? The test at least
-  // makes sure we don't produce a hidden symbol for foo<zed>::bar.
+  // definition. It's not really clear what we can do here, because we
+  // produce the symbols before even seeing the DEFAULT definition of zed.
+  // FIXME: Maybe the best thing to do here is error?  It's certainly hard
+  // to argue that this ought to be valid.
   template<typename T>
   struct DEFAULT foo {
     void bar() {}
@@ -742,7 +738,7 @@ namespace test35 {
   class DEFAULT zed {
   };
   // CHECK: define weak_odr void @_ZN6test353fooINS_3zedEE3barEv
-  // CHECK-HIDDEN: define weak_odr void @_ZN6test353fooINS_3zedEE3barEv
+  // CHECK-HIDDEN: define weak_odr hidden void @_ZN6test353fooINS_3zedEE3barEv
 }
 
 namespace test36 {
@@ -821,8 +817,8 @@ namespace test42 {
   };
   void bar<foo>::zed() {
   }
-  // CHECK: define hidden void @_ZN6test423barINS_3fooEE3zedEv
-  // CHECK-HIDDEN: define hidden void @_ZN6test423barINS_3fooEE3zedEv
+  // CHECK: define void @_ZN6test423barINS_3fooEE3zedEv
+  // CHECK-HIDDEN: define void @_ZN6test423barINS_3fooEE3zedEv
 }
 
 namespace test43 {
@@ -834,8 +830,8 @@ namespace test43 {
   template <>
   DEFAULT void bar<foo>() {
   }
-  // CHECK: define hidden void @_ZN6test433barINS_3fooEEEvv
-  // CHECK-HIDDEN: define hidden void @_ZN6test433barINS_3fooEEEvv
+  // CHECK: define void @_ZN6test433barINS_3fooEEEvv
+  // CHECK-HIDDEN: define void @_ZN6test433barINS_3fooEEEvv
 }
 
 namespace test44 {
@@ -1156,3 +1152,21 @@ namespace test62 {
   // gcc issues a warning about it being unused since "the type is already
   // defined". We should probably do the same.
 }
+
+namespace test63 {
+  enum HIDDEN E { E0 };
+  struct A {
+    template <E> static void foo() {}
+
+    template <E> struct B {
+      static void foo() {}
+    };
+  };
+
+  void test() {
+    A::foo<E0>();
+    A::B<E0>::foo();
+  }
+  // CHECK: define linkonce_odr hidden void @_ZN6test631A3fooILNS_1EE0EEEvv()
+  // CHECK: define linkonce_odr hidden void @_ZN6test631A1BILNS_1EE0EE3fooEv()
+}