[clang][NFC] Unify MacroState isAmbiguous and getModuleInfo

[davidstone]

Every call to MacroState::getModuleInfo, and MacroState::isAmbiguous are paired in the same function. Rather than doing the same work twice, just unify them into a single function, getModuleInfo, that returns both pieces of information in a new type ModuleMacroInfo.

Unfortunately, getModuleInfo andModuleMacroInfo already exist, so rename them to getFullModuleInfo and FullModuleMacroInfo, respectively, since the new type is a subset of the old type. The new type contains just the pieces consumers care about.

While we're there, use the range constructor of llvm::DenseSet instead of default constructing and calling insert in a loop.

[llvmorg-github-actions]

@llvm/pr-subscribers-clang

Author: David Stone (davidstone)

Changes

Every call to MacroState::getModuleInfo, and MacroState::isAmbiguous are paired in the same function. Rather than doing the same work twice, just unify them into a single function, getModuleInfo, that returns both pieces of information in a new type ModuleMacroInfo.

Unfortunately, getModuleInfo andModuleMacroInfo already exist, so rename them to getFullModuleInfo and FullModuleMacroInfo, respectively, since the new type is a subset of the old type. The new type contains just the pieces consumers care about.

While we're there, use the range constructor of llvm::DenseSet instead of default constructing and calling insert in a loop.

---

Full diff: https://github.com/llvm/llvm-project/pull/197867.diff

3 Files Affected:

• (modified) clang/include/clang/Lex/MacroInfo.h (+8-3)
• (modified) clang/include/clang/Lex/Preprocessor.h (+23-30)
• (modified) clang/lib/Lex/PPMacroExpansion.cpp (+6-6)

diff --git a/clang/include/clang/Lex/MacroInfo.h b/clang/include/clang/Lex/MacroInfo.h
index 19a706216d509..cd47a7048f2c6 100644
--- a/clang/include/clang/Lex/MacroInfo.h
+++ b/clang/include/clang/Lex/MacroInfo.h
@@ -583,6 +583,11 @@ class ModuleMacro : public llvm::FoldingSetNode {
   unsigned getNumOverridingMacros() const { return NumOverriddenBy; }
 };
 
+struct ModuleMacroInfo {
+  ArrayRef<ModuleMacro *> ActiveModuleMacros = {};
+  bool IsAmbiguous = false;
+};
+
 /// A description of the current definition of a macro.
 ///
 /// The definition of a macro comprises a set of (at least one) defining
@@ -593,9 +598,9 @@ class MacroDefinition {
 
 public:
   MacroDefinition() = default;
-  MacroDefinition(DefMacroDirective *MD, ArrayRef<ModuleMacro *> MMs,
-                  bool IsAmbiguous)
-      : LatestLocalAndAmbiguous(MD, IsAmbiguous), ModuleMacros(MMs) {}
+  MacroDefinition(DefMacroDirective *MD, ModuleMacroInfo Info)
+      : LatestLocalAndAmbiguous(MD, Info.IsAmbiguous),
+        ModuleMacros(Info.ActiveModuleMacros) {}
 
   /// Determine whether there is a definition of this macro.
   explicit operator bool() const {
diff --git a/clang/include/clang/Lex/Preprocessor.h b/clang/include/clang/Lex/Preprocessor.h
index c0c94b7ea4101..eb9e46e7cee89 100644
--- a/clang/include/clang/Lex/Preprocessor.h
+++ b/clang/include/clang/Lex/Preprocessor.h
@@ -874,7 +874,7 @@ class Preprocessor {
   SmallVector<MacroExpandsInfo, 2> DelayedMacroExpandsCallbacks;
 
   /// Information about a name that has been used to define a module macro.
-  struct ModuleMacroInfo {
+  struct FullModuleMacroInfo {
     /// The most recent macro directive for this identifier.
     MacroDirective *MD;
 
@@ -891,15 +891,15 @@ class Preprocessor {
     /// The module macros that are overridden by this macro.
     llvm::TinyPtrVector<ModuleMacro *> OverriddenMacros;
 
-    ModuleMacroInfo(MacroDirective *MD) : MD(MD) {}
+    FullModuleMacroInfo(MacroDirective *MD) : MD(MD) {}
   };
 
   /// The state of a macro for an identifier.
   class MacroState {
-    mutable llvm::PointerUnion<MacroDirective *, ModuleMacroInfo *> State;
+    mutable llvm::PointerUnion<MacroDirective *, FullModuleMacroInfo *> State;
 
-    ModuleMacroInfo *getModuleInfo(Preprocessor &PP,
-                                   const IdentifierInfo *II) const {
+    FullModuleMacroInfo *getFullModuleInfo(Preprocessor &PP,
+                                           const IdentifierInfo *II) const {
       if (II->isOutOfDate())
         PP.updateOutOfDateIdentifier(*II);
       // FIXME: Find a spare bit on IdentifierInfo and store a
@@ -910,10 +910,10 @@ class Preprocessor {
           !PP.CurSubmoduleState->VisibleModules.getGeneration())
         return nullptr;
 
-      auto *Info = dyn_cast_if_present<ModuleMacroInfo *>(State);
+      auto *Info = dyn_cast_if_present<FullModuleMacroInfo *>(State);
       if (!Info) {
         Info = new (PP.getPreprocessorAllocator())
-            ModuleMacroInfo(cast<MacroDirective *>(State));
+            FullModuleMacroInfo(cast<MacroDirective *>(State));
         State = Info;
       }
 
@@ -939,32 +939,27 @@ class Preprocessor {
     }
 
     ~MacroState() {
-      if (auto *Info = dyn_cast_if_present<ModuleMacroInfo *>(State))
-        Info->~ModuleMacroInfo();
+      if (auto *Info = dyn_cast_if_present<FullModuleMacroInfo *>(State))
+        Info->~FullModuleMacroInfo();
     }
 
     MacroDirective *getLatest() const {
-      if (auto *Info = dyn_cast_if_present<ModuleMacroInfo *>(State))
+      if (auto *Info = dyn_cast_if_present<FullModuleMacroInfo *>(State))
         return Info->MD;
       return cast<MacroDirective *>(State);
     }
 
     void setLatest(MacroDirective *MD) {
-      if (auto *Info = dyn_cast_if_present<ModuleMacroInfo *>(State))
+      if (auto *Info = dyn_cast_if_present<FullModuleMacroInfo *>(State))
         Info->MD = MD;
       else
         State = MD;
     }
 
-    bool isAmbiguous(Preprocessor &PP, const IdentifierInfo *II) const {
-      auto *Info = getModuleInfo(PP, II);
-      return Info ? Info->IsAmbiguous : false;
-    }
-
-    ArrayRef<ModuleMacro *>
-    getActiveModuleMacros(Preprocessor &PP, const IdentifierInfo *II) const {
-      if (auto *Info = getModuleInfo(PP, II))
-        return Info->ActiveModuleMacros;
+    ModuleMacroInfo getModuleInfo(Preprocessor &PP,
+                                  const IdentifierInfo *II) const {
+      if (auto *Info = getFullModuleInfo(PP, II))
+        return ModuleMacroInfo{Info->ActiveModuleMacros, Info->IsAmbiguous};
       return {};
     }
 
@@ -977,7 +972,7 @@ class Preprocessor {
     }
 
     void overrideActiveModuleMacros(Preprocessor &PP, IdentifierInfo *II) {
-      if (auto *Info = getModuleInfo(PP, II)) {
+      if (auto *Info = getFullModuleInfo(PP, II)) {
         Info->OverriddenMacros.insert(Info->OverriddenMacros.end(),
                                       Info->ActiveModuleMacros.begin(),
                                       Info->ActiveModuleMacros.end());
@@ -987,19 +982,19 @@ class Preprocessor {
     }
 
     ArrayRef<ModuleMacro*> getOverriddenMacros() const {
-      if (auto *Info = dyn_cast_if_present<ModuleMacroInfo *>(State))
+      if (auto *Info = dyn_cast_if_present<FullModuleMacroInfo *>(State))
         return Info->OverriddenMacros;
       return {};
     }
 
     void setOverriddenMacros(Preprocessor &PP,
                              ArrayRef<ModuleMacro *> Overrides) {
-      auto *Info = dyn_cast_if_present<ModuleMacroInfo *>(State);
+      auto *Info = dyn_cast_if_present<FullModuleMacroInfo *>(State);
       if (!Info) {
         if (Overrides.empty())
           return;
         Info = new (PP.getPreprocessorAllocator())
-            ModuleMacroInfo(cast<MacroDirective *>(State));
+            FullModuleMacroInfo(cast<MacroDirective *>(State));
         State = Info;
       }
       Info->OverriddenMacros.clear();
@@ -1423,8 +1418,7 @@ class Preprocessor {
     while (isa_and_nonnull<VisibilityMacroDirective>(MD))
       MD = MD->getPrevious();
     return MacroDefinition(dyn_cast_or_null<DefMacroDirective>(MD),
-                           S.getActiveModuleMacros(*this, II),
-                           S.isAmbiguous(*this, II));
+                           S.getModuleInfo(*this, II));
   }
 
   MacroDefinition getMacroDefinitionAtLoc(const IdentifierInfo *II,
@@ -1437,9 +1431,7 @@ class Preprocessor {
     if (auto *MD = S.getLatest())
       DI = MD->findDirectiveAtLoc(Loc, getSourceManager());
     // FIXME: Compute the set of active module macros at the specified location.
-    return MacroDefinition(DI.getDirective(),
-                           S.getActiveModuleMacros(*this, II),
-                           S.isAmbiguous(*this, II));
+    return MacroDefinition(DI.getDirective(), S.getModuleInfo(*this, II));
   }
 
   /// Given an identifier, return its latest non-imported MacroDirective
@@ -2619,7 +2611,8 @@ class Preprocessor {
 
   /// Update the set of active module macros and ambiguity flag for a module
   /// macro name.
-  void updateModuleMacroInfo(const IdentifierInfo *II, ModuleMacroInfo &Info);
+  void updateModuleMacroInfo(const IdentifierInfo *II,
+                             FullModuleMacroInfo &Info);
 
   DefMacroDirective *AllocateDefMacroDirective(MacroInfo *MI,
                                                SourceLocation Loc);
diff --git a/clang/lib/Lex/PPMacroExpansion.cpp b/clang/lib/Lex/PPMacroExpansion.cpp
index 20b8fe585a007..5fd3512d2f45c 100644
--- a/clang/lib/Lex/PPMacroExpansion.cpp
+++ b/clang/lib/Lex/PPMacroExpansion.cpp
@@ -176,7 +176,7 @@ ModuleMacro *Preprocessor::getModuleMacro(Module *Mod,
 }
 
 void Preprocessor::updateModuleMacroInfo(const IdentifierInfo *II,
-                                         ModuleMacroInfo &Info) {
+                                         FullModuleMacroInfo &Info) {
   assert(Info.ActiveModuleMacrosGeneration !=
              CurSubmoduleState->VisibleModules.getGeneration() &&
          "don't need to update this macro name info");
@@ -264,7 +264,9 @@ void Preprocessor::dumpMacroInfo(const IdentifierInfo *II) {
     State = &Pos->second;
 
   llvm::errs() << "MacroState " << State << " " << II->getNameStart();
-  if (State && State->isAmbiguous(*this, II))
+  const auto ModuleInfo =
+      State ? State->getModuleInfo(*this, II) : ModuleMacroInfo{};
+  if (ModuleInfo.IsAmbiguous)
     llvm::errs() << " ambiguous";
   if (State && !State->getOverriddenMacros().empty()) {
     llvm::errs() << " overrides";
@@ -281,10 +283,8 @@ void Preprocessor::dumpMacroInfo(const IdentifierInfo *II) {
   }
 
   // Dump module macros.
-  llvm::DenseSet<ModuleMacro*> Active;
-  for (auto *MM : State ? State->getActiveModuleMacros(*this, II)
-                        : ArrayRef<ModuleMacro *>())
-    Active.insert(MM);
+  llvm::DenseSet<ModuleMacro *> Active(llvm::from_range,
+                                       ModuleInfo.ActiveModuleMacros);
   llvm::DenseSet<ModuleMacro*> Visited;
   llvm::SmallVector<ModuleMacro *, 16> Worklist(Leaf);
   while (!Worklist.empty()) {

[cor3ntin]

LGTM