[NFC] Make MultiplexExternalSemaSource own sources

llvm-beanz · llvm-beanz · commit 5b5329bd41ba · 2022-09-02T13:57:39.000-05:00
This change refactors the MuiltiplexExternalSemaSource to take ownership of the underlying sources. As a result it makes a larger cleanup of external source ownership in Sema and the ChainedIncludesSource. Reviewed By: aaron.ballman, aprantl Differential Revision: https://reviews.llvm.org/D133158
diff --git a/clang-tools-extra/clang-include-fixer/IncludeFixer.cpp b/clang-tools-extra/clang-include-fixer/IncludeFixer.cpp
@@ -27,13 +27,14 @@ namespace {
 class Action : public clang::ASTFrontendAction {
 public:
   explicit Action(SymbolIndexManager &SymbolIndexMgr, bool MinimizeIncludePaths)
-      : SemaSource(SymbolIndexMgr, MinimizeIncludePaths,
-                   /*GenerateDiagnostics=*/false) {}
+      : SemaSource(new IncludeFixerSemaSource(SymbolIndexMgr,
+                                              MinimizeIncludePaths,
+                                              /*GenerateDiagnostics=*/false)) {}
 
   std::unique_ptr<clang::ASTConsumer>
   CreateASTConsumer(clang::CompilerInstance &Compiler,
                     StringRef InFile) override {
-    SemaSource.setFilePath(InFile);
+    SemaSource->setFilePath(InFile);
     return std::make_unique<clang::ASTConsumer>();
   }
 
@@ -51,8 +52,8 @@ class Action : public clang::ASTFrontendAction {
       CompletionConsumer = &Compiler->getCodeCompletionConsumer();
 
     Compiler->createSema(getTranslationUnitKind(), CompletionConsumer);
-    SemaSource.setCompilerInstance(Compiler);
-    Compiler->getSema().addExternalSource(&SemaSource);
+    SemaSource->setCompilerInstance(Compiler);
+    Compiler->getSema().addExternalSource(SemaSource.get());
 
     clang::ParseAST(Compiler->getSema(), Compiler->getFrontendOpts().ShowStats,
                     Compiler->getFrontendOpts().SkipFunctionBodies);
@@ -61,12 +62,12 @@ class Action : public clang::ASTFrontendAction {
   IncludeFixerContext
   getIncludeFixerContext(const clang::SourceManager &SourceManager,
                          clang::HeaderSearch &HeaderSearch) const {
-    return SemaSource.getIncludeFixerContext(SourceManager, HeaderSearch,
-                                             SemaSource.getMatchedSymbols());
+    return SemaSource->getIncludeFixerContext(SourceManager, HeaderSearch,
+                                              SemaSource->getMatchedSymbols());
   }
 
 private:
-  IncludeFixerSemaSource SemaSource;
+  IntrusiveRefCntPtr<IncludeFixerSemaSource> SemaSource;
 };
 
 } // namespace
diff --git a/clang/include/clang/Sema/MultiplexExternalSemaSource.h b/clang/include/clang/Sema/MultiplexExternalSemaSource.h
@@ -40,25 +40,24 @@ class MultiplexExternalSemaSource : public ExternalSemaSource {
   static char ID;
 
 private:
-  SmallVector<ExternalSemaSource *, 2> Sources; // doesn't own them.
+  SmallVector<ExternalSemaSource *, 2> Sources;
 
 public:
-
-  ///Constructs a new multiplexing external sema source and appends the
+  /// Constructs a new multiplexing external sema source and appends the
   /// given element to it.
   ///
-  ///\param[in] s1 - A non-null (old) ExternalSemaSource.
-  ///\param[in] s2 - A non-null (new) ExternalSemaSource.
+  ///\param[in] S1 - A non-null (old) ExternalSemaSource.
+  ///\param[in] S2 - A non-null (new) ExternalSemaSource.
   ///
-  MultiplexExternalSemaSource(ExternalSemaSource& s1, ExternalSemaSource& s2);
+  MultiplexExternalSemaSource(ExternalSemaSource *S1, ExternalSemaSource *S2);
 
   ~MultiplexExternalSemaSource() override;
 
-  ///Appends new source to the source list.
+  /// Appends new source to the source list.
   ///
   ///\param[in] source - An ExternalSemaSource.
   ///
-  void addSource(ExternalSemaSource &source);
+  void AddSource(ExternalSemaSource *Source);
 
   //===--------------------------------------------------------------------===//
   // ExternalASTSource.
diff --git a/clang/include/clang/Sema/Sema.h b/clang/include/clang/Sema/Sema.h
@@ -360,10 +360,7 @@ class Sema final {
   void operator=(const Sema &) = delete;
 
   ///Source of additional semantic information.
-  ExternalSemaSource *ExternalSource;
-
-  ///Whether Sema has generated a multiplexer and has to delete it.
-  bool isMultiplexExternalSource;
+  IntrusiveRefCntPtr<ExternalSemaSource> ExternalSource;
 
   static bool mightHaveNonExternalLinkage(const DeclaratorDecl *FD);
 
@@ -1637,7 +1634,7 @@ class Sema final {
   ASTContext &getASTContext() const { return Context; }
   ASTConsumer &getASTConsumer() const { return Consumer; }
   ASTMutationListener *getASTMutationListener() const;
-  ExternalSemaSource* getExternalSource() const { return ExternalSource; }
+  ExternalSemaSource *getExternalSource() const { return ExternalSource.get(); }
 
   DarwinSDKInfo *getDarwinSDKInfoForAvailabilityChecking(SourceLocation Loc,
                                                          StringRef Platform);
diff --git a/clang/lib/Frontend/ChainedIncludesSource.cpp b/clang/lib/Frontend/ChainedIncludesSource.cpp
@@ -27,15 +27,15 @@
 using namespace clang;
 
 namespace {
-class ChainedIncludesSourceImpl : public ExternalSemaSource {
+class ChainedIncludesSource : public ExternalSemaSource {
 public:
-  ChainedIncludesSourceImpl(std::vector<std::unique_ptr<CompilerInstance>> CIs)
+  ChainedIncludesSource(std::vector<std::unique_ptr<CompilerInstance>> CIs)
       : CIs(std::move(CIs)) {}
 
 protected:
-  //===----------------------------------------------------------------------===//
+  //===--------------------------------------------------------------------===//
   // ExternalASTSource interface.
-  //===----------------------------------------------------------------------===//
+  //===--------------------------------------------------------------------===//
 
   /// Return the amount of memory used by memory buffers, breaking down
   /// by heap-backed versus mmap'ed memory.
@@ -51,30 +51,7 @@ class ChainedIncludesSourceImpl : public ExternalSemaSource {
 private:
   std::vector<std::unique_ptr<CompilerInstance>> CIs;
 };
-
-/// Members of ChainedIncludesSource, factored out so we can initialize
-/// them before we initialize the ExternalSemaSource base class.
-struct ChainedIncludesSourceMembers {
-  ChainedIncludesSourceMembers(
-      std::vector<std::unique_ptr<CompilerInstance>> CIs,
-      IntrusiveRefCntPtr<ExternalSemaSource> FinalReader)
-      : Impl(std::move(CIs)), FinalReader(std::move(FinalReader)) {}
-  ChainedIncludesSourceImpl Impl;
-  IntrusiveRefCntPtr<ExternalSemaSource> FinalReader;
-};
-
-/// Use MultiplexExternalSemaSource to dispatch all ExternalSemaSource
-/// calls to the final reader.
-class ChainedIncludesSource
-    : private ChainedIncludesSourceMembers,
-      public MultiplexExternalSemaSource {
-public:
-  ChainedIncludesSource(std::vector<std::unique_ptr<CompilerInstance>> CIs,
-                        IntrusiveRefCntPtr<ExternalSemaSource> FinalReader)
-      : ChainedIncludesSourceMembers(std::move(CIs), std::move(FinalReader)),
-        MultiplexExternalSemaSource(Impl, *this->FinalReader) {}
-};
-}
+} // end anonymous namespace
 
 static ASTReader *
 createASTReader(CompilerInstance &CI, StringRef pchFile,
@@ -214,6 +191,8 @@ IntrusiveRefCntPtr<ExternalSemaSource> clang::createChainedIncludesSource(
   if (!Reader)
     return nullptr;
 
-  return IntrusiveRefCntPtr<ChainedIncludesSource>(
-      new ChainedIncludesSource(std::move(CIs), Reader));
+  auto ChainedSrc =
+      llvm::makeIntrusiveRefCnt<ChainedIncludesSource>(std::move(CIs));
+  return llvm::makeIntrusiveRefCnt<MultiplexExternalSemaSource>(
+      ChainedSrc.get(), Reader.get());
 }
diff --git a/clang/lib/Sema/MultiplexExternalSemaSource.cpp b/clang/lib/Sema/MultiplexExternalSemaSource.cpp
@@ -16,24 +16,30 @@ using namespace clang;
 
 char MultiplexExternalSemaSource::ID;
 
-///Constructs a new multiplexing external sema source and appends the
+/// Constructs a new multiplexing external sema source and appends the
 /// given element to it.
 ///
-MultiplexExternalSemaSource::MultiplexExternalSemaSource(ExternalSemaSource &s1,
-                                                        ExternalSemaSource &s2){
-  Sources.push_back(&s1);
-  Sources.push_back(&s2);
+MultiplexExternalSemaSource::MultiplexExternalSemaSource(
+    ExternalSemaSource *S1, ExternalSemaSource *S2) {
+  S1->Retain();
+  S2->Retain();
+  Sources.push_back(S1);
+  Sources.push_back(S2);
 }
 
 // pin the vtable here.
-MultiplexExternalSemaSource::~MultiplexExternalSemaSource() {}
+MultiplexExternalSemaSource::~MultiplexExternalSemaSource() {
+  for (auto *S : Sources)
+    S->Release();
+}
 
-///Appends new source to the source list.
+/// Appends new source to the source list.
 ///
 ///\param[in] source - An ExternalSemaSource.
 ///
-void MultiplexExternalSemaSource::addSource(ExternalSemaSource &source) {
-  Sources.push_back(&source);
+void MultiplexExternalSemaSource::AddSource(ExternalSemaSource *Source) {
+  Source->Retain();
+  Sources.push_back(Source);
 }
 
 //===----------------------------------------------------------------------===//
diff --git a/clang/lib/Sema/Sema.cpp b/clang/lib/Sema/Sema.cpp
@@ -185,11 +185,10 @@ const uint64_t Sema::MaximumAlignment;
 
 Sema::Sema(Preprocessor &pp, ASTContext &ctxt, ASTConsumer &consumer,
            TranslationUnitKind TUKind, CodeCompleteConsumer *CodeCompleter)
-    : ExternalSource(nullptr), isMultiplexExternalSource(false),
-      CurFPFeatures(pp.getLangOpts()), LangOpts(pp.getLangOpts()), PP(pp),
-      Context(ctxt), Consumer(consumer), Diags(PP.getDiagnostics()),
-      SourceMgr(PP.getSourceManager()), CollectStats(false),
-      CodeCompleter(CodeCompleter), CurContext(nullptr),
+    : ExternalSource(nullptr), CurFPFeatures(pp.getLangOpts()),
+      LangOpts(pp.getLangOpts()), PP(pp), Context(ctxt), Consumer(consumer),
+      Diags(PP.getDiagnostics()), SourceMgr(PP.getSourceManager()),
+      CollectStats(false), CodeCompleter(CodeCompleter), CurContext(nullptr),
       OriginalLexicalContext(nullptr), MSStructPragmaOn(false),
       MSPointerToMemberRepresentationMethod(
           LangOpts.getMSPointerToMemberRepresentationMethod()),
@@ -473,10 +472,6 @@ Sema::~Sema() {
         = dyn_cast_or_null<ExternalSemaSource>(Context.getExternalSource()))
     ExternalSema->ForgetSema();
 
-  // If Sema's ExternalSource is the multiplexer - we own it.
-  if (isMultiplexExternalSource)
-    delete ExternalSource;
-
   // Delete cached satisfactions.
   std::vector<ConstraintSatisfaction *> Satisfactions;
   Satisfactions.reserve(Satisfactions.size());
@@ -550,12 +545,10 @@ void Sema::addExternalSource(ExternalSemaSource *E) {
     return;
   }
 
-  if (isMultiplexExternalSource)
-    static_cast<MultiplexExternalSemaSource*>(ExternalSource)->addSource(*E);
-  else {
-    ExternalSource = new MultiplexExternalSemaSource(*ExternalSource, *E);
-    isMultiplexExternalSource = true;
-  }
+  if (auto *Ex = dyn_cast<MultiplexExternalSemaSource>(ExternalSource))
+    Ex->AddSource(E);
+  else
+    ExternalSource = new MultiplexExternalSemaSource(ExternalSource.get(), E);
 }
 
 /// Print out statistics about the semantic analysis.
@@ -1291,8 +1284,8 @@ void Sema::ActOnEndOfTranslationUnit() {
   //   translation unit, with an initializer equal to 0.
   llvm::SmallSet<VarDecl *, 32> Seen;
   for (TentativeDefinitionsType::iterator
-            T = TentativeDefinitions.begin(ExternalSource),
-         TEnd = TentativeDefinitions.end();
+           T = TentativeDefinitions.begin(ExternalSource.get()),
+           TEnd = TentativeDefinitions.end();
        T != TEnd; ++T) {
     VarDecl *VD = (*T)->getActingDefinition();
 
@@ -1336,8 +1329,9 @@ void Sema::ActOnEndOfTranslationUnit() {
   if (!Diags.hasErrorOccurred() && TUKind != TU_Module) {
     // Output warning for unused file scoped decls.
     for (UnusedFileScopedDeclsType::iterator
-           I = UnusedFileScopedDecls.begin(ExternalSource),
-           E = UnusedFileScopedDecls.end(); I != E; ++I) {
+             I = UnusedFileScopedDecls.begin(ExternalSource.get()),
+             E = UnusedFileScopedDecls.end();
+         I != E; ++I) {
       if (ShouldRemoveFromUnused(this, *I))
         continue;
 
diff --git a/clang/lib/Sema/SemaDeclCXX.cpp b/clang/lib/Sema/SemaDeclCXX.cpp
@@ -18183,8 +18183,8 @@ void Sema::CheckDelegatingCtorCycles() {
   llvm::SmallPtrSet<CXXConstructorDecl*, 4> Valid, Invalid, Current;
 
   for (DelegatingCtorDeclsType::iterator
-         I = DelegatingCtorDecls.begin(ExternalSource),
-         E = DelegatingCtorDecls.end();
+           I = DelegatingCtorDecls.begin(ExternalSource.get()),
+           E = DelegatingCtorDecls.end();
        I != E; ++I)
     DelegatingCycleHelper(*I, Valid, Invalid, Current, *this);
 
diff --git a/clang/unittests/Sema/ExternalSemaSourceTest.cpp b/clang/unittests/Sema/ExternalSemaSourceTest.cpp
