Skip to content

[clang][NFC] Refactor clang::Linkage #71049

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 4 commits into from
Nov 2, 2023
Merged

Conversation

Endilll
Copy link
Contributor

@Endilll Endilll commented Nov 2, 2023

This patch introduces a new enumerator Invalid = 0, shifting other enumerators by +1. Contrary to how it might sound, this actually affirms status quo of how this enum is stored in clang::Decl:

  /// If 0, we have not computed the linkage of this declaration.
  /// Otherwise, it is the linkage + 1.
  mutable unsigned CacheValidAndLinkage : 3;

This patch makes debuggers to not be mistaken about enumerator stored in this bit-field. It also converts clang::Linkage to a scoped enum.

This patch introduces a new enumerator `Invalid = 0`, shifting other enumerators by +1. Contrary to how it might sound, this actually affirms status quo of how this enum is stored in `clang::Decl`:
```
  /// If 0, we have not computed the linkage of this declaration.
  /// Otherwise, it is the linkage + 1.
  mutable unsigned CacheValidAndLinkage : 3;
```
This patch makes debuggers to not be mistaken about enumerator stored in this bit-field. It also converts `clang::Linkage` to a scoped enum.
@llvmbot llvmbot added clang Clang issues not falling into any other category clangd clang:frontend Language frontend issues, e.g. anything involving "Sema" clang:modules C++20 modules and Clang Header Modules clang:codegen IR generation bugs: mangling, exceptions, etc. labels Nov 2, 2023
@llvmbot
Copy link
Member

llvmbot commented Nov 2, 2023

@llvm/pr-subscribers-clangd
@llvm/pr-subscribers-clang

@llvm/pr-subscribers-clang-modules

Author: Vlad Serebrennikov (Endilll)

Changes

This patch introduces a new enumerator Invalid = 0, shifting other enumerators by +1. Contrary to how it might sound, this actually affirms status quo of how this enum is stored in clang::Decl:

  /// If 0, we have not computed the linkage of this declaration.
  /// Otherwise, it is the linkage + 1.
  mutable unsigned CacheValidAndLinkage : 3;

This patch makes debuggers to not be mistaken about enumerator stored in this bit-field. It also converts clang::Linkage to a scoped enum.


Patch is 40.18 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/71049.diff

28 Files Affected:

  • (modified) clang-tools-extra/clang-doc/Serialize.cpp (+2-2)
  • (modified) clang-tools-extra/clangd/Quality.cpp (+2-1)
  • (modified) clang-tools-extra/clangd/SemanticHighlighting.cpp (+2-1)
  • (modified) clang/include/clang/AST/DeclBase.h (+5-6)
  • (modified) clang/include/clang/AST/Type.h (+1-1)
  • (modified) clang/include/clang/Basic/Linkage.h (+34-19)
  • (modified) clang/include/clang/Basic/Visibility.h (+15-13)
  • (modified) clang/lib/AST/APValue.cpp (+1-1)
  • (modified) clang/lib/AST/Decl.cpp (+20-8)
  • (modified) clang/lib/AST/ItaniumMangle.cpp (+2-2)
  • (modified) clang/lib/AST/MicrosoftMangle.cpp (+2-3)
  • (modified) clang/lib/AST/Type.cpp (+4-4)
  • (modified) clang/lib/CodeGen/CodeGenModule.cpp (+2-2)
  • (modified) clang/lib/CodeGen/ItaniumCXXABI.cpp (+9-6)
  • (modified) clang/lib/CodeGen/MicrosoftCXXABI.cpp (+9-6)
  • (modified) clang/lib/Index/IndexSymbol.cpp (+11-9)
  • (modified) clang/lib/Sema/SemaChecking.cpp (+1-1)
  • (modified) clang/lib/Sema/SemaDecl.cpp (+10-14)
  • (modified) clang/lib/Sema/SemaExpr.cpp (+1-1)
  • (modified) clang/lib/Sema/SemaModule.cpp (+3-3)
  • (modified) clang/lib/Sema/SemaOverload.cpp (+1-1)
  • (modified) clang/lib/Sema/SemaTemplate.cpp (+2-2)
  • (modified) clang/lib/Serialization/ASTReaderDecl.cpp (+2-2)
  • (modified) clang/lib/Serialization/ASTWriterDecl.cpp (+3-3)
  • (modified) clang/tools/clang-extdef-mapping/ClangExtDefMapGen.cpp (+5-3)
  • (modified) clang/tools/libclang/CIndex.cpp (+8-6)
  • (modified) clang/tools/libclang/CXIndexDataConsumer.cpp (+28-28)
  • (modified) clang/unittests/AST/DeclTest.cpp (+4-4)
diff --git a/clang-tools-extra/clang-doc/Serialize.cpp b/clang-tools-extra/clang-doc/Serialize.cpp
index ac8e253ac06ea0b..3b074d849e8a9cf 100644
--- a/clang-tools-extra/clang-doc/Serialize.cpp
+++ b/clang-tools-extra/clang-doc/Serialize.cpp
@@ -257,8 +257,8 @@ static bool isPublic(const clang::AccessSpecifier AS,
                      const clang::Linkage Link) {
   if (AS == clang::AccessSpecifier::AS_private)
     return false;
-  else if ((Link == clang::Linkage::ModuleLinkage) ||
-           (Link == clang::Linkage::ExternalLinkage))
+  else if ((Link == clang::Linkage::Module) ||
+           (Link == clang::Linkage::External))
     return true;
   return false; // otherwise, linkage is some form of internal linkage
 }
diff --git a/clang-tools-extra/clangd/Quality.cpp b/clang-tools-extra/clangd/Quality.cpp
index 8840f805f0e87c7..7371d95fbf27547 100644
--- a/clang-tools-extra/clangd/Quality.cpp
+++ b/clang-tools-extra/clangd/Quality.cpp
@@ -274,7 +274,8 @@ computeScope(const NamedDecl *D) {
     return SymbolRelevanceSignals::ClassScope;
   // ExternalLinkage threshold could be tweaked, e.g. module-visible as global.
   // Avoid caching linkage if it may change after enclosing code completion.
-  if (hasUnstableLinkage(D) || D->getLinkageInternal() < ExternalLinkage)
+  if (hasUnstableLinkage(D) || llvm::to_underlying(D->getLinkageInternal()) <
+                                   llvm::to_underlying(Linkage::External))
     return SymbolRelevanceSignals::FileScope;
   return SymbolRelevanceSignals::GlobalScope;
 }
diff --git a/clang-tools-extra/clangd/SemanticHighlighting.cpp b/clang-tools-extra/clangd/SemanticHighlighting.cpp
index 7649e37e1f96663..49e479abf456210 100644
--- a/clang-tools-extra/clangd/SemanticHighlighting.cpp
+++ b/clang-tools-extra/clangd/SemanticHighlighting.cpp
@@ -617,7 +617,8 @@ std::optional<HighlightingModifier> scopeModifier(const NamedDecl *D) {
   if (DC->isTranslationUnit() && D->isTemplateParameter())
     return std::nullopt;
   // ExternalLinkage threshold could be tweaked, e.g. module-visible as global.
-  if (D->getLinkageInternal() < ExternalLinkage)
+  if (llvm::to_underlying(D->getLinkageInternal()) <
+      llvm::to_underlying(Linkage::External))
     return HighlightingModifier::FileScope;
   return HighlightingModifier::GlobalScope;
 }
diff --git a/clang/include/clang/AST/DeclBase.h b/clang/include/clang/AST/DeclBase.h
index df1d6e8a3b5af72..f784fa73af5bad5 100644
--- a/clang/include/clang/AST/DeclBase.h
+++ b/clang/include/clang/AST/DeclBase.h
@@ -49,7 +49,7 @@ class ExternalSourceSymbolAttr;
 class FunctionDecl;
 class FunctionType;
 class IdentifierInfo;
-enum Linkage : unsigned char;
+enum class Linkage : unsigned char;
 class LinkageSpecDecl;
 class Module;
 class NamedDecl;
@@ -335,7 +335,6 @@ class alignas(8) Decl {
   unsigned IdentifierNamespace : 14;
 
   /// If 0, we have not computed the linkage of this declaration.
-  /// Otherwise, it is the linkage + 1.
   mutable unsigned CacheValidAndLinkage : 3;
 
   /// Allocate memory for a deserialized declaration.
@@ -386,7 +385,7 @@ class alignas(8) Decl {
         Implicit(false), Used(false), Referenced(false),
         TopLevelDeclInObjCContainer(false), Access(AS_none), FromASTFile(0),
         IdentifierNamespace(getIdentifierNamespaceForKind(DK)),
-        CacheValidAndLinkage(0) {
+        CacheValidAndLinkage(llvm::to_underlying(Linkage::Invalid)) {
     if (StatisticsEnabled) add(DK);
   }
 
@@ -395,7 +394,7 @@ class alignas(8) Decl {
         Used(false), Referenced(false), TopLevelDeclInObjCContainer(false),
         Access(AS_none), FromASTFile(0),
         IdentifierNamespace(getIdentifierNamespaceForKind(DK)),
-        CacheValidAndLinkage(0) {
+        CacheValidAndLinkage(llvm::to_underlying(Linkage::Invalid)) {
     if (StatisticsEnabled) add(DK);
   }
 
@@ -405,11 +404,11 @@ class alignas(8) Decl {
   void updateOutOfDate(IdentifierInfo &II) const;
 
   Linkage getCachedLinkage() const {
-    return Linkage(CacheValidAndLinkage - 1);
+    return static_cast<Linkage>(CacheValidAndLinkage);
   }
 
   void setCachedLinkage(Linkage L) const {
-    CacheValidAndLinkage = L + 1;
+    CacheValidAndLinkage = llvm::to_underlying(L);
   }
 
   bool hasCachedLinkage() const {
diff --git a/clang/include/clang/AST/Type.h b/clang/include/clang/AST/Type.h
index 23022d7e1a72928..96c12a84e139f40 100644
--- a/clang/include/clang/AST/Type.h
+++ b/clang/include/clang/AST/Type.h
@@ -1996,7 +1996,7 @@ class alignas(TypeAlignment) Type : public ExtQualsTypeCommonBase {
     TypeBits.Dependence = static_cast<unsigned>(Dependence);
     TypeBits.CacheValid = false;
     TypeBits.CachedLocalOrUnnamed = false;
-    TypeBits.CachedLinkage = NoLinkage;
+    TypeBits.CachedLinkage = llvm::to_underlying(Linkage::None);
     TypeBits.FromAST = false;
   }
 
diff --git a/clang/include/clang/Basic/Linkage.h b/clang/include/clang/Basic/Linkage.h
index 0b7b61954a075ae..9cf36e522947fa8 100644
--- a/clang/include/clang/Basic/Linkage.h
+++ b/clang/include/clang/Basic/Linkage.h
@@ -14,21 +14,25 @@
 #ifndef LLVM_CLANG_BASIC_LINKAGE_H
 #define LLVM_CLANG_BASIC_LINKAGE_H
 
+#include "llvm/Support/ErrorHandling.h"
 #include <utility>
 
 namespace clang {
 
 /// Describes the different kinds of linkage
 /// (C++ [basic.link], C99 6.2.2) that an entity may have.
-enum Linkage : unsigned char {
+enum class Linkage : unsigned char {
+  // Linkage hasn't been computed.
+  Invalid = 0,
+
   /// No linkage, which means that the entity is unique and
   /// can only be referred to from within its scope.
-  NoLinkage = 0,
+  None,
 
   /// Internal linkage, which indicates that the entity can
   /// be referred to from within the translation unit (but not other
   /// translation units).
-  InternalLinkage,
+  Internal,
 
   /// External linkage within a unique namespace.
   ///
@@ -37,21 +41,21 @@ enum Linkage : unsigned char {
   /// their names are unique to this translation unit, which is
   /// equivalent to having internal linkage from the code-generation
   /// point of view.
-  UniqueExternalLinkage,
+  UniqueExternal,
 
   /// No linkage according to the standard, but is visible from other
   /// translation units because of types defined in a inline function.
-  VisibleNoLinkage,
+  VisibleNone,
 
   /// Module linkage, which indicates that the entity can be referred
   /// to from other translation units within the same module, and indirectly
   /// from arbitrary other translation units through inline functions and
   /// templates in the module interface.
-  ModuleLinkage,
+  Module,
 
   /// External linkage, which indicates that the entity can
   /// be referred to from other translation units.
-  ExternalLinkage
+  External
 };
 
 /// Describes the different kinds of language linkage
@@ -84,22 +88,33 @@ inline bool isUniqueGVALinkage(GVALinkage L) {
 }
 
 inline bool isExternallyVisible(Linkage L) {
-  return L >= VisibleNoLinkage;
+  switch (L) {
+  case Linkage::Invalid:
+    llvm_unreachable("Linkage hasn't been computed!");
+  case Linkage::None:
+  case Linkage::Internal:
+  case Linkage::UniqueExternal:
+    return false;
+  case Linkage::VisibleNone:
+  case Linkage::Module:
+  case Linkage::External:
+    return true;
+  }
 }
 
 inline Linkage getFormalLinkage(Linkage L) {
   switch (L) {
-  case UniqueExternalLinkage:
-    return ExternalLinkage;
-  case VisibleNoLinkage:
-    return NoLinkage;
+  case Linkage::UniqueExternal:
+    return Linkage::External;
+  case Linkage::VisibleNone:
+    return Linkage::None;
   default:
     return L;
   }
 }
 
 inline bool isExternalFormalLinkage(Linkage L) {
-  return getFormalLinkage(L) == ExternalLinkage;
+  return getFormalLinkage(L) == Linkage::External;
 }
 
 /// Compute the minimum linkage given two linkages.
@@ -111,13 +126,13 @@ inline bool isExternalFormalLinkage(Linkage L) {
 /// special cases for when VisibleNoLinkage would lose the visible bit and
 /// become NoLinkage.
 inline Linkage minLinkage(Linkage L1, Linkage L2) {
-  if (L2 == VisibleNoLinkage)
+  if (L2 == Linkage::VisibleNone)
     std::swap(L1, L2);
-  if (L1 == VisibleNoLinkage) {
-    if (L2 == InternalLinkage)
-      return NoLinkage;
-    if (L2 == UniqueExternalLinkage)
-      return NoLinkage;
+  if (L1 == Linkage::VisibleNone) {
+    if (L2 == Linkage::Internal)
+      return Linkage::None;
+    if (L2 == Linkage::UniqueExternal)
+      return Linkage::None;
   }
   return L1 < L2 ? L1 : L2;
 }
diff --git a/clang/include/clang/Basic/Visibility.h b/clang/include/clang/Basic/Visibility.h
index 57d9754ae4a9858..1e196300be421eb 100644
--- a/clang/include/clang/Basic/Visibility.h
+++ b/clang/include/clang/Basic/Visibility.h
@@ -15,6 +15,7 @@
 #define LLVM_CLANG_BASIC_VISIBILITY_H
 
 #include "clang/Basic/Linkage.h"
+#include "llvm/ADT/STLForwardCompat.h"
 #include <cassert>
 #include <cstdint>
 
@@ -56,10 +57,11 @@ class LinkageInfo {
 
   void setVisibility(Visibility V, bool E) { visibility_ = V; explicit_ = E; }
 public:
-  LinkageInfo() : linkage_(ExternalLinkage), visibility_(DefaultVisibility),
-                  explicit_(false) {}
+  LinkageInfo()
+      : linkage_(llvm::to_underlying(Linkage::External)),
+        visibility_(DefaultVisibility), explicit_(false) {}
   LinkageInfo(Linkage L, Visibility V, bool E)
-    : linkage_(L), visibility_(V), explicit_(E) {
+      : linkage_(llvm::to_underlying(L)), visibility_(V), explicit_(E) {
     assert(getLinkage() == L && getVisibility() == V &&
            isVisibilityExplicit() == E && "Enum truncated!");
   }
@@ -68,23 +70,23 @@ class LinkageInfo {
     return LinkageInfo();
   }
   static LinkageInfo internal() {
-    return LinkageInfo(InternalLinkage, DefaultVisibility, false);
+    return LinkageInfo(Linkage::Internal, DefaultVisibility, false);
   }
   static LinkageInfo uniqueExternal() {
-    return LinkageInfo(UniqueExternalLinkage, DefaultVisibility, false);
+    return LinkageInfo(Linkage::UniqueExternal, DefaultVisibility, false);
   }
   static LinkageInfo none() {
-    return LinkageInfo(NoLinkage, DefaultVisibility, false);
+    return LinkageInfo(Linkage::None, DefaultVisibility, false);
   }
   static LinkageInfo visible_none() {
-    return LinkageInfo(VisibleNoLinkage, DefaultVisibility, false);
+    return LinkageInfo(Linkage::VisibleNone, DefaultVisibility, false);
   }
 
-  Linkage getLinkage() const { return (Linkage)linkage_; }
+  Linkage getLinkage() const { return static_cast<Linkage>(linkage_); }
   Visibility getVisibility() const { return (Visibility)visibility_; }
   bool isVisibilityExplicit() const { return explicit_; }
 
-  void setLinkage(Linkage L) { linkage_ = L; }
+  void setLinkage(Linkage L) { linkage_ = llvm::to_underlying(L); }
 
   void mergeLinkage(Linkage L) {
     setLinkage(minLinkage(getLinkage(), L));
@@ -96,10 +98,10 @@ class LinkageInfo {
   void mergeExternalVisibility(Linkage L) {
     Linkage ThisL = getLinkage();
     if (!isExternallyVisible(L)) {
-      if (ThisL == VisibleNoLinkage)
-        ThisL = NoLinkage;
-      else if (ThisL == ExternalLinkage)
-        ThisL = UniqueExternalLinkage;
+      if (ThisL == Linkage::VisibleNone)
+        ThisL = Linkage::None;
+      else if (ThisL == Linkage::External)
+        ThisL = Linkage::UniqueExternal;
     }
     setLinkage(ThisL);
   }
diff --git a/clang/lib/AST/APValue.cpp b/clang/lib/AST/APValue.cpp
index d08c2936b56dd45..4eae308ef5b34c6 100644
--- a/clang/lib/AST/APValue.cpp
+++ b/clang/lib/AST/APValue.cpp
@@ -1115,7 +1115,7 @@ LinkageInfo LinkageComputer::getLVForValue(const APValue &V,
 
   auto MergeLV = [&](LinkageInfo MergeLV) {
     LV.merge(MergeLV);
-    return LV.getLinkage() == InternalLinkage;
+    return LV.getLinkage() == Linkage::Internal;
   };
   auto Merge = [&](const APValue &V) {
     return MergeLV(getLVForValue(V, computation));
diff --git a/clang/lib/AST/Decl.cpp b/clang/lib/AST/Decl.cpp
index 6efc177d61c03ba..e8062b680fbc3ab 100644
--- a/clang/lib/AST/Decl.cpp
+++ b/clang/lib/AST/Decl.cpp
@@ -1204,12 +1204,11 @@ Linkage NamedDecl::getFormalLinkage() const {
   // [basic.namespace.general]/p2
   //   A namespace is never attached to a named module and never has a name with
   //   module linkage.
-  if (isInModulePurview(this) &&
-      InternalLinkage == ExternalLinkage &&
+  if (isInModulePurview(this) && InternalLinkage == Linkage::External &&
       !isExportedFromModuleInterfaceUnit(
           cast<NamedDecl>(this->getCanonicalDecl())) &&
       !isa<NamespaceDecl>(this))
-    InternalLinkage = ModuleLinkage;
+    InternalLinkage = Linkage::Module;
 
   return clang::getFormalLinkage(InternalLinkage);
 }
@@ -1337,7 +1336,7 @@ LinkageInfo LinkageComputer::getLVForClosure(const DeclContext *DC,
   // visible, then the lambda is too. We apply the same rules to blocks.
   if (!isExternallyVisible(OwnerLV.getLinkage()))
     return LinkageInfo::none();
-  return LinkageInfo(VisibleNoLinkage, OwnerLV.getVisibility(),
+  return LinkageInfo(Linkage::VisibleNone, OwnerLV.getVisibility(),
                      OwnerLV.isVisibilityExplicit());
 }
 
@@ -1382,7 +1381,7 @@ LinkageInfo LinkageComputer::getLVForLocalDecl(const NamedDecl *D,
 
       if (const VarDecl *Prev = Var->getPreviousDecl()) {
         LinkageInfo PrevLV = getLVForDecl(Prev, computation);
-        if (PrevLV.getLinkage())
+        if (PrevLV.getLinkage() != Linkage::Invalid)
           LV.setLinkage(PrevLV.getLinkage());
         LV.mergeVisibility(PrevLV);
       }
@@ -1433,14 +1432,14 @@ LinkageInfo LinkageComputer::getLVForLocalDecl(const NamedDecl *D,
             computation.isValueVisibility()
                 ? Context.getLangOpts().getValueVisibilityMode()
                 : Context.getLangOpts().getTypeVisibilityMode();
-        return LinkageInfo(VisibleNoLinkage, globalVisibility,
+        return LinkageInfo(Linkage::VisibleNone, globalVisibility,
                            /*visibilityExplicit=*/false);
       }
     }
   }
   if (!isExternallyVisible(LV.getLinkage()))
     return LinkageInfo::none();
-  return LinkageInfo(VisibleNoLinkage, LV.getVisibility(),
+  return LinkageInfo(Linkage::VisibleNone, LV.getVisibility(),
                      LV.isVisibilityExplicit());
 }
 
@@ -1921,7 +1920,20 @@ bool NamedDecl::declarationReplaces(NamedDecl *OldD, bool IsKnownNewer) const {
 }
 
 bool NamedDecl::hasLinkage() const {
-  return getFormalLinkage() != NoLinkage;
+  switch (getFormalLinkage()) {
+  case Linkage::Invalid:
+    llvm_unreachable("Linkage hasn't been computed!");
+  case Linkage::None:
+    return false;
+  case Linkage::Internal:
+    return true;
+  case Linkage::UniqueExternal:
+  case Linkage::VisibleNone:
+    llvm_unreachable("Non-formal linkage is not allowed here!");
+  case Linkage::Module:
+  case Linkage::External:
+    return true;
+  }
 }
 
 NamedDecl *NamedDecl::getUnderlyingDeclImpl() {
diff --git a/clang/lib/AST/ItaniumMangle.cpp b/clang/lib/AST/ItaniumMangle.cpp
index 261a56c4b666ae5..8530675ca2a1ce2 100644
--- a/clang/lib/AST/ItaniumMangle.cpp
+++ b/clang/lib/AST/ItaniumMangle.cpp
@@ -708,7 +708,7 @@ ItaniumMangleContextImpl::getEffectiveDeclContext(const Decl *D) {
 }
 
 bool ItaniumMangleContextImpl::isInternalLinkageDecl(const NamedDecl *ND) {
-  if (ND && ND->getFormalLinkage() == InternalLinkage &&
+  if (ND && ND->getFormalLinkage() == Linkage::Internal &&
       !ND->isExternallyVisible() &&
       getEffectiveDeclContext(ND)->isFileContext() &&
       !ND->isInAnonymousNamespace())
@@ -790,7 +790,7 @@ bool ItaniumMangleContextImpl::shouldMangleCXXName(const NamedDecl *D) {
     if (DC->isFunctionOrMethod() && D->hasLinkage())
       while (!DC->isFileContext())
         DC = getEffectiveParentContext(DC);
-    if (DC->isTranslationUnit() && D->getFormalLinkage() != InternalLinkage &&
+    if (DC->isTranslationUnit() && D->getFormalLinkage() != Linkage::Internal &&
         !CXXNameMangler::shouldHaveAbiTags(*this, VD) &&
         !isa<VarTemplateSpecializationDecl>(VD) &&
         !VD->getOwningModuleForLinkage())
diff --git a/clang/lib/AST/MicrosoftMangle.cpp b/clang/lib/AST/MicrosoftMangle.cpp
index f6d8cdee8443d59..b7cfcbc9dfa4677 100644
--- a/clang/lib/AST/MicrosoftMangle.cpp
+++ b/clang/lib/AST/MicrosoftMangle.cpp
@@ -539,9 +539,8 @@ bool MicrosoftMangleContextImpl::shouldMangleCXXName(const NamedDecl *D) {
       while (!DC->isNamespace() && !DC->isTranslationUnit())
         DC = getEffectiveParentContext(DC);
 
-    if (DC->isTranslationUnit() && D->getFormalLinkage() == InternalLinkage &&
-        !isa<VarTemplateSpecializationDecl>(D) &&
-        D->getIdentifier() != nullptr)
+    if (DC->isTranslationUnit() && D->getFormalLinkage() == Linkage::Internal &&
+        !isa<VarTemplateSpecializationDecl>(D) && D->getIdentifier() != nullptr)
       return false;
   }
 
diff --git a/clang/lib/AST/Type.cpp b/clang/lib/AST/Type.cpp
index 98a4f12c4f574fa..d1cbfbd150ba53f 100644
--- a/clang/lib/AST/Type.cpp
+++ b/clang/lib/AST/Type.cpp
@@ -4175,7 +4175,7 @@ template <class Private> class TypePropertyCache {
     // Compute the cached properties and then set the cache.
     CachedProperties Result = computeCachedProperties(T);
     T->TypeBits.CacheValid = true;
-    T->TypeBits.CachedLinkage = Result.getLinkage();
+    T->TypeBits.CachedLinkage = llvm::to_underlying(Result.getLinkage());
     T->TypeBits.CachedLocalOrUnnamed = Result.hasLocalOrUnnamedType();
   }
 };
@@ -4207,20 +4207,20 @@ static CachedProperties computeCachedProperties(const Type *T) {
     // Treat instantiation-dependent types as external.
     if (!T->isInstantiationDependentType()) T->dump();
     assert(T->isInstantiationDependentType());
-    return CachedProperties(ExternalLinkage, false);
+    return CachedProperties(Linkage::External, false);
 
   case Type::Auto:
   case Type::DeducedTemplateSpecialization:
     // Give non-deduced 'auto' types external linkage. We should only see them
     // here in error recovery.
-    return CachedProperties(ExternalLinkage, false);
+    return CachedProperties(Linkage::External, false);
 
   case Type::BitInt:
   case Type::Builtin:
     // C++ [basic.link]p8:
     //   A type is said to have linkage if and only if:
     //     - it is a fundamental type (3.9.1); or
-    return CachedProperties(ExternalLinkage, false);
+    return CachedProperties(Linkage::External, false);
 
   case Type::Record:
   case Type::Enum: {
diff --git a/clang/lib/CodeGen/CodeGenModule.cpp b/clang/lib/CodeGen/CodeGenModule.cpp
index cc81a68b15c4324..35f651b39f6748a 100644
--- a/clang/lib/CodeGen/CodeGenModule.cpp
+++ b/clang/lib/CodeGen/CodeGenModule.cpp
@@ -3997,7 +3997,7 @@ TargetMVPriority(const TargetInfo &TI,
 llvm::GlobalValue::LinkageTypes getMultiversionLinkage(CodeGenModule &CGM,
                                                        GlobalDecl GD) {
   const FunctionDecl *FD = cast<FunctionDecl>(GD.getDecl());
-  if (FD->getFormalLinkage() == InternalLinkage)
+  if (FD->getFormalLinkage() == Linkage::Internal)
     return llvm::GlobalValue::InternalLinkage;
   return llvm::GlobalValue::WeakODRLinkage;
 }
@@ -5051,7 +5051,7 @@ void CodeGenModule::MaybeHandleStaticInExternC(const SomeDecl *D,
     return;
 
   // Must have internal linkage and an ordinary name.
-  if (!D->getIdentifier() || D->getFormalLinkage() != InternalLinkage)
+  if (!D->getIdentifier() || D->getFormalLinkage() != Linkage::Internal)
     return;
 
   // Must be in an extern "C" context. Entities declared directly within
diff --git a/clang/lib/CodeGen/ItaniumCXXABI.cpp b/clang/lib/CodeGen/ItaniumCXXABI.cpp
index c7295b3144ed16e..55d2696750ae7a0 100644
--- a/clang/lib/CodeGen/ItaniumCXXABI.cpp
+++ b/clang/lib/CodeGen/ItaniumCXXABI.cpp
@@ -3706,14 +3706,17 @@ static llvm::GlobalVariable::LinkageTypes getTypeInfoLinkage(CodeGenModule &CGM,
     return llvm::GlobalValue::InternalLinkage;
 
   switch (Ty->getLinkage()) {
-  case NoLinkage:
-  case InternalLinkage:
-  case UniqueExternalLinkage:
+  case Linkage::Invalid:
+    llvm_unreachable("Linkage hasn't been computed!");
+
+  case Linkage::None:
+  case Linkage::Internal:
+  case Linkage::UniqueExternal:
     return llvm::GlobalValue::InternalLinkage;
 
-  case VisibleNoLinkage:
-  case ModuleLinkage:
-  case ExternalLinkage:
+  case Linkage::VisibleNone:
+  case Linkage::Module:
+  case Linkage::External:
     // RTTI is not enabled, which means that this type info struct is going
     // to be used for exception handling. Give it linkonce_odr linkage.
     if ...
[truncated]

@llvmbot
Copy link
Member

llvmbot commented Nov 2, 2023

@llvm/pr-subscribers-clang-codegen

Author: Vlad Serebrennikov (Endilll)

Changes

This patch introduces a new enumerator Invalid = 0, shifting other enumerators by +1. Contrary to how it might sound, this actually affirms status quo of how this enum is stored in clang::Decl:

  /// If 0, we have not computed the linkage of this declaration.
  /// Otherwise, it is the linkage + 1.
  mutable unsigned CacheValidAndLinkage : 3;

This patch makes debuggers to not be mistaken about enumerator stored in this bit-field. It also converts clang::Linkage to a scoped enum.


Patch is 40.18 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/71049.diff

28 Files Affected:

  • (modified) clang-tools-extra/clang-doc/Serialize.cpp (+2-2)
  • (modified) clang-tools-extra/clangd/Quality.cpp (+2-1)
  • (modified) clang-tools-extra/clangd/SemanticHighlighting.cpp (+2-1)
  • (modified) clang/include/clang/AST/DeclBase.h (+5-6)
  • (modified) clang/include/clang/AST/Type.h (+1-1)
  • (modified) clang/include/clang/Basic/Linkage.h (+34-19)
  • (modified) clang/include/clang/Basic/Visibility.h (+15-13)
  • (modified) clang/lib/AST/APValue.cpp (+1-1)
  • (modified) clang/lib/AST/Decl.cpp (+20-8)
  • (modified) clang/lib/AST/ItaniumMangle.cpp (+2-2)
  • (modified) clang/lib/AST/MicrosoftMangle.cpp (+2-3)
  • (modified) clang/lib/AST/Type.cpp (+4-4)
  • (modified) clang/lib/CodeGen/CodeGenModule.cpp (+2-2)
  • (modified) clang/lib/CodeGen/ItaniumCXXABI.cpp (+9-6)
  • (modified) clang/lib/CodeGen/MicrosoftCXXABI.cpp (+9-6)
  • (modified) clang/lib/Index/IndexSymbol.cpp (+11-9)
  • (modified) clang/lib/Sema/SemaChecking.cpp (+1-1)
  • (modified) clang/lib/Sema/SemaDecl.cpp (+10-14)
  • (modified) clang/lib/Sema/SemaExpr.cpp (+1-1)
  • (modified) clang/lib/Sema/SemaModule.cpp (+3-3)
  • (modified) clang/lib/Sema/SemaOverload.cpp (+1-1)
  • (modified) clang/lib/Sema/SemaTemplate.cpp (+2-2)
  • (modified) clang/lib/Serialization/ASTReaderDecl.cpp (+2-2)
  • (modified) clang/lib/Serialization/ASTWriterDecl.cpp (+3-3)
  • (modified) clang/tools/clang-extdef-mapping/ClangExtDefMapGen.cpp (+5-3)
  • (modified) clang/tools/libclang/CIndex.cpp (+8-6)
  • (modified) clang/tools/libclang/CXIndexDataConsumer.cpp (+28-28)
  • (modified) clang/unittests/AST/DeclTest.cpp (+4-4)
diff --git a/clang-tools-extra/clang-doc/Serialize.cpp b/clang-tools-extra/clang-doc/Serialize.cpp
index ac8e253ac06ea0b..3b074d849e8a9cf 100644
--- a/clang-tools-extra/clang-doc/Serialize.cpp
+++ b/clang-tools-extra/clang-doc/Serialize.cpp
@@ -257,8 +257,8 @@ static bool isPublic(const clang::AccessSpecifier AS,
                      const clang::Linkage Link) {
   if (AS == clang::AccessSpecifier::AS_private)
     return false;
-  else if ((Link == clang::Linkage::ModuleLinkage) ||
-           (Link == clang::Linkage::ExternalLinkage))
+  else if ((Link == clang::Linkage::Module) ||
+           (Link == clang::Linkage::External))
     return true;
   return false; // otherwise, linkage is some form of internal linkage
 }
diff --git a/clang-tools-extra/clangd/Quality.cpp b/clang-tools-extra/clangd/Quality.cpp
index 8840f805f0e87c7..7371d95fbf27547 100644
--- a/clang-tools-extra/clangd/Quality.cpp
+++ b/clang-tools-extra/clangd/Quality.cpp
@@ -274,7 +274,8 @@ computeScope(const NamedDecl *D) {
     return SymbolRelevanceSignals::ClassScope;
   // ExternalLinkage threshold could be tweaked, e.g. module-visible as global.
   // Avoid caching linkage if it may change after enclosing code completion.
-  if (hasUnstableLinkage(D) || D->getLinkageInternal() < ExternalLinkage)
+  if (hasUnstableLinkage(D) || llvm::to_underlying(D->getLinkageInternal()) <
+                                   llvm::to_underlying(Linkage::External))
     return SymbolRelevanceSignals::FileScope;
   return SymbolRelevanceSignals::GlobalScope;
 }
diff --git a/clang-tools-extra/clangd/SemanticHighlighting.cpp b/clang-tools-extra/clangd/SemanticHighlighting.cpp
index 7649e37e1f96663..49e479abf456210 100644
--- a/clang-tools-extra/clangd/SemanticHighlighting.cpp
+++ b/clang-tools-extra/clangd/SemanticHighlighting.cpp
@@ -617,7 +617,8 @@ std::optional<HighlightingModifier> scopeModifier(const NamedDecl *D) {
   if (DC->isTranslationUnit() && D->isTemplateParameter())
     return std::nullopt;
   // ExternalLinkage threshold could be tweaked, e.g. module-visible as global.
-  if (D->getLinkageInternal() < ExternalLinkage)
+  if (llvm::to_underlying(D->getLinkageInternal()) <
+      llvm::to_underlying(Linkage::External))
     return HighlightingModifier::FileScope;
   return HighlightingModifier::GlobalScope;
 }
diff --git a/clang/include/clang/AST/DeclBase.h b/clang/include/clang/AST/DeclBase.h
index df1d6e8a3b5af72..f784fa73af5bad5 100644
--- a/clang/include/clang/AST/DeclBase.h
+++ b/clang/include/clang/AST/DeclBase.h
@@ -49,7 +49,7 @@ class ExternalSourceSymbolAttr;
 class FunctionDecl;
 class FunctionType;
 class IdentifierInfo;
-enum Linkage : unsigned char;
+enum class Linkage : unsigned char;
 class LinkageSpecDecl;
 class Module;
 class NamedDecl;
@@ -335,7 +335,6 @@ class alignas(8) Decl {
   unsigned IdentifierNamespace : 14;
 
   /// If 0, we have not computed the linkage of this declaration.
-  /// Otherwise, it is the linkage + 1.
   mutable unsigned CacheValidAndLinkage : 3;
 
   /// Allocate memory for a deserialized declaration.
@@ -386,7 +385,7 @@ class alignas(8) Decl {
         Implicit(false), Used(false), Referenced(false),
         TopLevelDeclInObjCContainer(false), Access(AS_none), FromASTFile(0),
         IdentifierNamespace(getIdentifierNamespaceForKind(DK)),
-        CacheValidAndLinkage(0) {
+        CacheValidAndLinkage(llvm::to_underlying(Linkage::Invalid)) {
     if (StatisticsEnabled) add(DK);
   }
 
@@ -395,7 +394,7 @@ class alignas(8) Decl {
         Used(false), Referenced(false), TopLevelDeclInObjCContainer(false),
         Access(AS_none), FromASTFile(0),
         IdentifierNamespace(getIdentifierNamespaceForKind(DK)),
-        CacheValidAndLinkage(0) {
+        CacheValidAndLinkage(llvm::to_underlying(Linkage::Invalid)) {
     if (StatisticsEnabled) add(DK);
   }
 
@@ -405,11 +404,11 @@ class alignas(8) Decl {
   void updateOutOfDate(IdentifierInfo &II) const;
 
   Linkage getCachedLinkage() const {
-    return Linkage(CacheValidAndLinkage - 1);
+    return static_cast<Linkage>(CacheValidAndLinkage);
   }
 
   void setCachedLinkage(Linkage L) const {
-    CacheValidAndLinkage = L + 1;
+    CacheValidAndLinkage = llvm::to_underlying(L);
   }
 
   bool hasCachedLinkage() const {
diff --git a/clang/include/clang/AST/Type.h b/clang/include/clang/AST/Type.h
index 23022d7e1a72928..96c12a84e139f40 100644
--- a/clang/include/clang/AST/Type.h
+++ b/clang/include/clang/AST/Type.h
@@ -1996,7 +1996,7 @@ class alignas(TypeAlignment) Type : public ExtQualsTypeCommonBase {
     TypeBits.Dependence = static_cast<unsigned>(Dependence);
     TypeBits.CacheValid = false;
     TypeBits.CachedLocalOrUnnamed = false;
-    TypeBits.CachedLinkage = NoLinkage;
+    TypeBits.CachedLinkage = llvm::to_underlying(Linkage::None);
     TypeBits.FromAST = false;
   }
 
diff --git a/clang/include/clang/Basic/Linkage.h b/clang/include/clang/Basic/Linkage.h
index 0b7b61954a075ae..9cf36e522947fa8 100644
--- a/clang/include/clang/Basic/Linkage.h
+++ b/clang/include/clang/Basic/Linkage.h
@@ -14,21 +14,25 @@
 #ifndef LLVM_CLANG_BASIC_LINKAGE_H
 #define LLVM_CLANG_BASIC_LINKAGE_H
 
+#include "llvm/Support/ErrorHandling.h"
 #include <utility>
 
 namespace clang {
 
 /// Describes the different kinds of linkage
 /// (C++ [basic.link], C99 6.2.2) that an entity may have.
-enum Linkage : unsigned char {
+enum class Linkage : unsigned char {
+  // Linkage hasn't been computed.
+  Invalid = 0,
+
   /// No linkage, which means that the entity is unique and
   /// can only be referred to from within its scope.
-  NoLinkage = 0,
+  None,
 
   /// Internal linkage, which indicates that the entity can
   /// be referred to from within the translation unit (but not other
   /// translation units).
-  InternalLinkage,
+  Internal,
 
   /// External linkage within a unique namespace.
   ///
@@ -37,21 +41,21 @@ enum Linkage : unsigned char {
   /// their names are unique to this translation unit, which is
   /// equivalent to having internal linkage from the code-generation
   /// point of view.
-  UniqueExternalLinkage,
+  UniqueExternal,
 
   /// No linkage according to the standard, but is visible from other
   /// translation units because of types defined in a inline function.
-  VisibleNoLinkage,
+  VisibleNone,
 
   /// Module linkage, which indicates that the entity can be referred
   /// to from other translation units within the same module, and indirectly
   /// from arbitrary other translation units through inline functions and
   /// templates in the module interface.
-  ModuleLinkage,
+  Module,
 
   /// External linkage, which indicates that the entity can
   /// be referred to from other translation units.
-  ExternalLinkage
+  External
 };
 
 /// Describes the different kinds of language linkage
@@ -84,22 +88,33 @@ inline bool isUniqueGVALinkage(GVALinkage L) {
 }
 
 inline bool isExternallyVisible(Linkage L) {
-  return L >= VisibleNoLinkage;
+  switch (L) {
+  case Linkage::Invalid:
+    llvm_unreachable("Linkage hasn't been computed!");
+  case Linkage::None:
+  case Linkage::Internal:
+  case Linkage::UniqueExternal:
+    return false;
+  case Linkage::VisibleNone:
+  case Linkage::Module:
+  case Linkage::External:
+    return true;
+  }
 }
 
 inline Linkage getFormalLinkage(Linkage L) {
   switch (L) {
-  case UniqueExternalLinkage:
-    return ExternalLinkage;
-  case VisibleNoLinkage:
-    return NoLinkage;
+  case Linkage::UniqueExternal:
+    return Linkage::External;
+  case Linkage::VisibleNone:
+    return Linkage::None;
   default:
     return L;
   }
 }
 
 inline bool isExternalFormalLinkage(Linkage L) {
-  return getFormalLinkage(L) == ExternalLinkage;
+  return getFormalLinkage(L) == Linkage::External;
 }
 
 /// Compute the minimum linkage given two linkages.
@@ -111,13 +126,13 @@ inline bool isExternalFormalLinkage(Linkage L) {
 /// special cases for when VisibleNoLinkage would lose the visible bit and
 /// become NoLinkage.
 inline Linkage minLinkage(Linkage L1, Linkage L2) {
-  if (L2 == VisibleNoLinkage)
+  if (L2 == Linkage::VisibleNone)
     std::swap(L1, L2);
-  if (L1 == VisibleNoLinkage) {
-    if (L2 == InternalLinkage)
-      return NoLinkage;
-    if (L2 == UniqueExternalLinkage)
-      return NoLinkage;
+  if (L1 == Linkage::VisibleNone) {
+    if (L2 == Linkage::Internal)
+      return Linkage::None;
+    if (L2 == Linkage::UniqueExternal)
+      return Linkage::None;
   }
   return L1 < L2 ? L1 : L2;
 }
diff --git a/clang/include/clang/Basic/Visibility.h b/clang/include/clang/Basic/Visibility.h
index 57d9754ae4a9858..1e196300be421eb 100644
--- a/clang/include/clang/Basic/Visibility.h
+++ b/clang/include/clang/Basic/Visibility.h
@@ -15,6 +15,7 @@
 #define LLVM_CLANG_BASIC_VISIBILITY_H
 
 #include "clang/Basic/Linkage.h"
+#include "llvm/ADT/STLForwardCompat.h"
 #include <cassert>
 #include <cstdint>
 
@@ -56,10 +57,11 @@ class LinkageInfo {
 
   void setVisibility(Visibility V, bool E) { visibility_ = V; explicit_ = E; }
 public:
-  LinkageInfo() : linkage_(ExternalLinkage), visibility_(DefaultVisibility),
-                  explicit_(false) {}
+  LinkageInfo()
+      : linkage_(llvm::to_underlying(Linkage::External)),
+        visibility_(DefaultVisibility), explicit_(false) {}
   LinkageInfo(Linkage L, Visibility V, bool E)
-    : linkage_(L), visibility_(V), explicit_(E) {
+      : linkage_(llvm::to_underlying(L)), visibility_(V), explicit_(E) {
     assert(getLinkage() == L && getVisibility() == V &&
            isVisibilityExplicit() == E && "Enum truncated!");
   }
@@ -68,23 +70,23 @@ class LinkageInfo {
     return LinkageInfo();
   }
   static LinkageInfo internal() {
-    return LinkageInfo(InternalLinkage, DefaultVisibility, false);
+    return LinkageInfo(Linkage::Internal, DefaultVisibility, false);
   }
   static LinkageInfo uniqueExternal() {
-    return LinkageInfo(UniqueExternalLinkage, DefaultVisibility, false);
+    return LinkageInfo(Linkage::UniqueExternal, DefaultVisibility, false);
   }
   static LinkageInfo none() {
-    return LinkageInfo(NoLinkage, DefaultVisibility, false);
+    return LinkageInfo(Linkage::None, DefaultVisibility, false);
   }
   static LinkageInfo visible_none() {
-    return LinkageInfo(VisibleNoLinkage, DefaultVisibility, false);
+    return LinkageInfo(Linkage::VisibleNone, DefaultVisibility, false);
   }
 
-  Linkage getLinkage() const { return (Linkage)linkage_; }
+  Linkage getLinkage() const { return static_cast<Linkage>(linkage_); }
   Visibility getVisibility() const { return (Visibility)visibility_; }
   bool isVisibilityExplicit() const { return explicit_; }
 
-  void setLinkage(Linkage L) { linkage_ = L; }
+  void setLinkage(Linkage L) { linkage_ = llvm::to_underlying(L); }
 
   void mergeLinkage(Linkage L) {
     setLinkage(minLinkage(getLinkage(), L));
@@ -96,10 +98,10 @@ class LinkageInfo {
   void mergeExternalVisibility(Linkage L) {
     Linkage ThisL = getLinkage();
     if (!isExternallyVisible(L)) {
-      if (ThisL == VisibleNoLinkage)
-        ThisL = NoLinkage;
-      else if (ThisL == ExternalLinkage)
-        ThisL = UniqueExternalLinkage;
+      if (ThisL == Linkage::VisibleNone)
+        ThisL = Linkage::None;
+      else if (ThisL == Linkage::External)
+        ThisL = Linkage::UniqueExternal;
     }
     setLinkage(ThisL);
   }
diff --git a/clang/lib/AST/APValue.cpp b/clang/lib/AST/APValue.cpp
index d08c2936b56dd45..4eae308ef5b34c6 100644
--- a/clang/lib/AST/APValue.cpp
+++ b/clang/lib/AST/APValue.cpp
@@ -1115,7 +1115,7 @@ LinkageInfo LinkageComputer::getLVForValue(const APValue &V,
 
   auto MergeLV = [&](LinkageInfo MergeLV) {
     LV.merge(MergeLV);
-    return LV.getLinkage() == InternalLinkage;
+    return LV.getLinkage() == Linkage::Internal;
   };
   auto Merge = [&](const APValue &V) {
     return MergeLV(getLVForValue(V, computation));
diff --git a/clang/lib/AST/Decl.cpp b/clang/lib/AST/Decl.cpp
index 6efc177d61c03ba..e8062b680fbc3ab 100644
--- a/clang/lib/AST/Decl.cpp
+++ b/clang/lib/AST/Decl.cpp
@@ -1204,12 +1204,11 @@ Linkage NamedDecl::getFormalLinkage() const {
   // [basic.namespace.general]/p2
   //   A namespace is never attached to a named module and never has a name with
   //   module linkage.
-  if (isInModulePurview(this) &&
-      InternalLinkage == ExternalLinkage &&
+  if (isInModulePurview(this) && InternalLinkage == Linkage::External &&
       !isExportedFromModuleInterfaceUnit(
           cast<NamedDecl>(this->getCanonicalDecl())) &&
       !isa<NamespaceDecl>(this))
-    InternalLinkage = ModuleLinkage;
+    InternalLinkage = Linkage::Module;
 
   return clang::getFormalLinkage(InternalLinkage);
 }
@@ -1337,7 +1336,7 @@ LinkageInfo LinkageComputer::getLVForClosure(const DeclContext *DC,
   // visible, then the lambda is too. We apply the same rules to blocks.
   if (!isExternallyVisible(OwnerLV.getLinkage()))
     return LinkageInfo::none();
-  return LinkageInfo(VisibleNoLinkage, OwnerLV.getVisibility(),
+  return LinkageInfo(Linkage::VisibleNone, OwnerLV.getVisibility(),
                      OwnerLV.isVisibilityExplicit());
 }
 
@@ -1382,7 +1381,7 @@ LinkageInfo LinkageComputer::getLVForLocalDecl(const NamedDecl *D,
 
       if (const VarDecl *Prev = Var->getPreviousDecl()) {
         LinkageInfo PrevLV = getLVForDecl(Prev, computation);
-        if (PrevLV.getLinkage())
+        if (PrevLV.getLinkage() != Linkage::Invalid)
           LV.setLinkage(PrevLV.getLinkage());
         LV.mergeVisibility(PrevLV);
       }
@@ -1433,14 +1432,14 @@ LinkageInfo LinkageComputer::getLVForLocalDecl(const NamedDecl *D,
             computation.isValueVisibility()
                 ? Context.getLangOpts().getValueVisibilityMode()
                 : Context.getLangOpts().getTypeVisibilityMode();
-        return LinkageInfo(VisibleNoLinkage, globalVisibility,
+        return LinkageInfo(Linkage::VisibleNone, globalVisibility,
                            /*visibilityExplicit=*/false);
       }
     }
   }
   if (!isExternallyVisible(LV.getLinkage()))
     return LinkageInfo::none();
-  return LinkageInfo(VisibleNoLinkage, LV.getVisibility(),
+  return LinkageInfo(Linkage::VisibleNone, LV.getVisibility(),
                      LV.isVisibilityExplicit());
 }
 
@@ -1921,7 +1920,20 @@ bool NamedDecl::declarationReplaces(NamedDecl *OldD, bool IsKnownNewer) const {
 }
 
 bool NamedDecl::hasLinkage() const {
-  return getFormalLinkage() != NoLinkage;
+  switch (getFormalLinkage()) {
+  case Linkage::Invalid:
+    llvm_unreachable("Linkage hasn't been computed!");
+  case Linkage::None:
+    return false;
+  case Linkage::Internal:
+    return true;
+  case Linkage::UniqueExternal:
+  case Linkage::VisibleNone:
+    llvm_unreachable("Non-formal linkage is not allowed here!");
+  case Linkage::Module:
+  case Linkage::External:
+    return true;
+  }
 }
 
 NamedDecl *NamedDecl::getUnderlyingDeclImpl() {
diff --git a/clang/lib/AST/ItaniumMangle.cpp b/clang/lib/AST/ItaniumMangle.cpp
index 261a56c4b666ae5..8530675ca2a1ce2 100644
--- a/clang/lib/AST/ItaniumMangle.cpp
+++ b/clang/lib/AST/ItaniumMangle.cpp
@@ -708,7 +708,7 @@ ItaniumMangleContextImpl::getEffectiveDeclContext(const Decl *D) {
 }
 
 bool ItaniumMangleContextImpl::isInternalLinkageDecl(const NamedDecl *ND) {
-  if (ND && ND->getFormalLinkage() == InternalLinkage &&
+  if (ND && ND->getFormalLinkage() == Linkage::Internal &&
       !ND->isExternallyVisible() &&
       getEffectiveDeclContext(ND)->isFileContext() &&
       !ND->isInAnonymousNamespace())
@@ -790,7 +790,7 @@ bool ItaniumMangleContextImpl::shouldMangleCXXName(const NamedDecl *D) {
     if (DC->isFunctionOrMethod() && D->hasLinkage())
       while (!DC->isFileContext())
         DC = getEffectiveParentContext(DC);
-    if (DC->isTranslationUnit() && D->getFormalLinkage() != InternalLinkage &&
+    if (DC->isTranslationUnit() && D->getFormalLinkage() != Linkage::Internal &&
         !CXXNameMangler::shouldHaveAbiTags(*this, VD) &&
         !isa<VarTemplateSpecializationDecl>(VD) &&
         !VD->getOwningModuleForLinkage())
diff --git a/clang/lib/AST/MicrosoftMangle.cpp b/clang/lib/AST/MicrosoftMangle.cpp
index f6d8cdee8443d59..b7cfcbc9dfa4677 100644
--- a/clang/lib/AST/MicrosoftMangle.cpp
+++ b/clang/lib/AST/MicrosoftMangle.cpp
@@ -539,9 +539,8 @@ bool MicrosoftMangleContextImpl::shouldMangleCXXName(const NamedDecl *D) {
       while (!DC->isNamespace() && !DC->isTranslationUnit())
         DC = getEffectiveParentContext(DC);
 
-    if (DC->isTranslationUnit() && D->getFormalLinkage() == InternalLinkage &&
-        !isa<VarTemplateSpecializationDecl>(D) &&
-        D->getIdentifier() != nullptr)
+    if (DC->isTranslationUnit() && D->getFormalLinkage() == Linkage::Internal &&
+        !isa<VarTemplateSpecializationDecl>(D) && D->getIdentifier() != nullptr)
       return false;
   }
 
diff --git a/clang/lib/AST/Type.cpp b/clang/lib/AST/Type.cpp
index 98a4f12c4f574fa..d1cbfbd150ba53f 100644
--- a/clang/lib/AST/Type.cpp
+++ b/clang/lib/AST/Type.cpp
@@ -4175,7 +4175,7 @@ template <class Private> class TypePropertyCache {
     // Compute the cached properties and then set the cache.
     CachedProperties Result = computeCachedProperties(T);
     T->TypeBits.CacheValid = true;
-    T->TypeBits.CachedLinkage = Result.getLinkage();
+    T->TypeBits.CachedLinkage = llvm::to_underlying(Result.getLinkage());
     T->TypeBits.CachedLocalOrUnnamed = Result.hasLocalOrUnnamedType();
   }
 };
@@ -4207,20 +4207,20 @@ static CachedProperties computeCachedProperties(const Type *T) {
     // Treat instantiation-dependent types as external.
     if (!T->isInstantiationDependentType()) T->dump();
     assert(T->isInstantiationDependentType());
-    return CachedProperties(ExternalLinkage, false);
+    return CachedProperties(Linkage::External, false);
 
   case Type::Auto:
   case Type::DeducedTemplateSpecialization:
     // Give non-deduced 'auto' types external linkage. We should only see them
     // here in error recovery.
-    return CachedProperties(ExternalLinkage, false);
+    return CachedProperties(Linkage::External, false);
 
   case Type::BitInt:
   case Type::Builtin:
     // C++ [basic.link]p8:
     //   A type is said to have linkage if and only if:
     //     - it is a fundamental type (3.9.1); or
-    return CachedProperties(ExternalLinkage, false);
+    return CachedProperties(Linkage::External, false);
 
   case Type::Record:
   case Type::Enum: {
diff --git a/clang/lib/CodeGen/CodeGenModule.cpp b/clang/lib/CodeGen/CodeGenModule.cpp
index cc81a68b15c4324..35f651b39f6748a 100644
--- a/clang/lib/CodeGen/CodeGenModule.cpp
+++ b/clang/lib/CodeGen/CodeGenModule.cpp
@@ -3997,7 +3997,7 @@ TargetMVPriority(const TargetInfo &TI,
 llvm::GlobalValue::LinkageTypes getMultiversionLinkage(CodeGenModule &CGM,
                                                        GlobalDecl GD) {
   const FunctionDecl *FD = cast<FunctionDecl>(GD.getDecl());
-  if (FD->getFormalLinkage() == InternalLinkage)
+  if (FD->getFormalLinkage() == Linkage::Internal)
     return llvm::GlobalValue::InternalLinkage;
   return llvm::GlobalValue::WeakODRLinkage;
 }
@@ -5051,7 +5051,7 @@ void CodeGenModule::MaybeHandleStaticInExternC(const SomeDecl *D,
     return;
 
   // Must have internal linkage and an ordinary name.
-  if (!D->getIdentifier() || D->getFormalLinkage() != InternalLinkage)
+  if (!D->getIdentifier() || D->getFormalLinkage() != Linkage::Internal)
     return;
 
   // Must be in an extern "C" context. Entities declared directly within
diff --git a/clang/lib/CodeGen/ItaniumCXXABI.cpp b/clang/lib/CodeGen/ItaniumCXXABI.cpp
index c7295b3144ed16e..55d2696750ae7a0 100644
--- a/clang/lib/CodeGen/ItaniumCXXABI.cpp
+++ b/clang/lib/CodeGen/ItaniumCXXABI.cpp
@@ -3706,14 +3706,17 @@ static llvm::GlobalVariable::LinkageTypes getTypeInfoLinkage(CodeGenModule &CGM,
     return llvm::GlobalValue::InternalLinkage;
 
   switch (Ty->getLinkage()) {
-  case NoLinkage:
-  case InternalLinkage:
-  case UniqueExternalLinkage:
+  case Linkage::Invalid:
+    llvm_unreachable("Linkage hasn't been computed!");
+
+  case Linkage::None:
+  case Linkage::Internal:
+  case Linkage::UniqueExternal:
     return llvm::GlobalValue::InternalLinkage;
 
-  case VisibleNoLinkage:
-  case ModuleLinkage:
-  case ExternalLinkage:
+  case Linkage::VisibleNone:
+  case Linkage::Module:
+  case Linkage::External:
     // RTTI is not enabled, which means that this type info struct is going
     // to be used for exception handling. Give it linkonce_odr linkage.
     if ...
[truncated]

@@ -2214,7 +2214,7 @@ void ASTWriter::WriteDeclAbbrevs() {
Abv->Add(BitCodeAbbrevOp(0)); // TSCSpec
Abv->Add(BitCodeAbbrevOp(0)); // InitStyle
Abv->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::Fixed, 1)); // isARCPseudoStrong
Abv->Add(BitCodeAbbrevOp(0)); // Linkage
Abv->Add(BitCodeAbbrevOp(1)); // Linkage
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ChuanqiXu9 confirmed that touching this is fine (thank you!). If I guess correctly that this value corresponds to Linkage::None we have in some default constructors, we should consider changing that default to Linkage::Invalid.

Linkage::None being default feels like we computed the linkage and arrived at Linkage::None even when it's not the case.

CC @AaronBallman

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed that those should be swapped to Invalid instead of None (better done as a separate follow-up patch as it isn't an NFC change). However, this is a change to the serialization format so a new PCH consumed by an older Clang will misbehave. Please check whether VERSION_MAJOR (in ASTBitCodes.h) has been updated for this release and if it hasn't been, increment it by one.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I guess correctly that this value corresponds to Linkage::None we have in some default constructors, we should consider changing that default to Linkage::Invalid.

No. It is not the case. Sorry for didn't explaining things clearly. The value here means the linkage for ParmVarDecl according to line 2185. So it is natural that ParmVarDecl doesn't have a linkage.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ohhhh! Thanks. None makes far more sense now. Still a change to the serialization values though, so we need to check whether we should do a version bump or not.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please check whether VERSION_MAJOR (in ASTBitCodes.h) has been updated for this release and if it hasn't been, increment it by one.

This has been bumped after 17 cut-off at least once: 6fb08d8

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Excellent, thank you! Hmmm, we should probably put a comment there like "this value was last bumped for the Clang NN release" so we don't have to do so much work in the future... (Not as part of these changes, just thinking out loud.)

@@ -2214,7 +2214,7 @@ void ASTWriter::WriteDeclAbbrevs() {
Abv->Add(BitCodeAbbrevOp(0)); // TSCSpec
Abv->Add(BitCodeAbbrevOp(0)); // InitStyle
Abv->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::Fixed, 1)); // isARCPseudoStrong
Abv->Add(BitCodeAbbrevOp(0)); // Linkage
Abv->Add(BitCodeAbbrevOp(1)); // Linkage
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed that those should be swapped to Invalid instead of None (better done as a separate follow-up patch as it isn't an NFC change). However, this is a change to the serialization format so a new PCH consumed by an older Clang will misbehave. Please check whether VERSION_MAJOR (in ASTBitCodes.h) has been updated for this release and if it hasn't been, increment it by one.

@Endilll Endilll changed the title [clang][NFC] Refactor clang::Linkage [clang] Refactor clang::Linkage Nov 2, 2023
Copy link
Member

@ChuanqiXu9 ChuanqiXu9 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. I didn't understand the full context in the private chat.

@@ -2214,7 +2214,7 @@ void ASTWriter::WriteDeclAbbrevs() {
Abv->Add(BitCodeAbbrevOp(0)); // TSCSpec
Abv->Add(BitCodeAbbrevOp(0)); // InitStyle
Abv->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::Fixed, 1)); // isARCPseudoStrong
Abv->Add(BitCodeAbbrevOp(0)); // Linkage
Abv->Add(BitCodeAbbrevOp(1)); // Linkage
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I guess correctly that this value corresponds to Linkage::None we have in some default constructors, we should consider changing that default to Linkage::Invalid.

No. It is not the case. Sorry for didn't explaining things clearly. The value here means the linkage for ParmVarDecl according to line 2185. So it is natural that ParmVarDecl doesn't have a linkage.

Copy link
Collaborator

@AaronBallman AaronBallman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM aside from the comment nit from @ChuanqiXu9

Copy link
Collaborator

@AaronBallman AaronBallman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM but please add NFC back into the patch title so it's more clear why there's no changes to test coverage. Thank you for the cleanup!

@Endilll Endilll changed the title [clang] Refactor clang::Linkage [clang][NFC] Refactor clang::Linkage Nov 2, 2023
@Endilll Endilll merged commit 8775947 into llvm:main Nov 2, 2023
@Endilll Endilll deleted the refactor-linkage branch November 2, 2023 16:57
Guzhu-AMD pushed a commit to GPUOpen-Drivers/llvm-project that referenced this pull request Nov 9, 2023
Local branch amd-gfx ab0d8a7 Merged main:7d77bbef4ad9 into amd-gfx:1a7ad991e1f8
Remote branch main 8775947 [clang][NFC] Refactor `clang::Linkage` (llvm#71049)
harbandana pushed a commit to harbandana/LLVM-REPO that referenced this pull request Jan 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
clang:codegen IR generation bugs: mangling, exceptions, etc. clang:frontend Language frontend issues, e.g. anything involving "Sema" clang:modules C++20 modules and Clang Header Modules clang Clang issues not falling into any other category clangd
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants