[NFC] Make MultiplexExternalSemaSource own sources

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
index 2af1f62..45fc156 100644 (file)
--- 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 @@ public:
       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 @@ public:
   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
index 78658dc..7049255 100644 (file)
--- 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
index bd81d3a..459c110 100644 (file)
--- 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 @@ public:
   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
index 380eba4..c1a9f25 100644 (file)
--- 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 @@ protected:
 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
index 0727756..55e0154 100644 (file)
--- 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
index 0410032..1158685 100644 (file)
--- 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
index 04d06c1..76db4e5 100644 (file)
--- 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
index b7679fe..88ab430 100644 (file)
--- a/clang/unittests/Sema/ExternalSemaSourceTest.cpp
+++ b/clang/unittests/Sema/ExternalSemaSourceTest.cpp
@@ -219,6 +219,8 @@ public:
   void PushWatcher(DiagnosticWatcher *Watcher) { Watchers.push_back(Watcher); }
 };
 
+using llvm::makeIntrusiveRefCnt;
+
 // Make sure that the DiagnosticWatcher is not miscounting.
 TEST(ExternalSemaSource, DiagCheck) {
   auto Installer = std::make_unique<ExternalSemaSourceInstaller>();
@@ -234,14 +236,14 @@ TEST(ExternalSemaSource, DiagCheck) {
 // instead of the usual suggestion we would use above.
 TEST(ExternalSemaSource, ExternalTypoCorrectionPrioritized) {
   auto Installer = std::make_unique<ExternalSemaSourceInstaller>();
-  NamespaceTypoProvider Provider("AAB", "BBB");
+  auto Provider = makeIntrusiveRefCnt<NamespaceTypoProvider>("AAB", "BBB");
   DiagnosticWatcher Watcher("AAB", "BBB");
-  Installer->PushSource(&Provider);
+  Installer->PushSource(Provider.get());
   Installer->PushWatcher(&Watcher);
   std::vector<std::string> Args(1, "-std=c++11");
   ASSERT_TRUE(clang::tooling::runToolOnCodeWithArgs(
       std::move(Installer), "namespace AAA { } using namespace AAB;", Args));
-  ASSERT_LE(0, Provider.CallCount);
+  ASSERT_LE(0, Provider->CallCount);
   ASSERT_EQ(1, Watcher.SeenCount);
 }
 
@@ -249,34 +251,34 @@ TEST(ExternalSemaSource, ExternalTypoCorrectionPrioritized) {
 // ExternalSemaSource.
 TEST(ExternalSemaSource, ExternalTypoCorrectionOrdering) {
   auto Installer = std::make_unique<ExternalSemaSourceInstaller>();
-  NamespaceTypoProvider First("XXX", "BBB");
-  NamespaceTypoProvider Second("AAB", "CCC");
-  NamespaceTypoProvider Third("AAB", "DDD");
+  auto First = makeIntrusiveRefCnt<NamespaceTypoProvider>("XXX", "BBB");
+  auto Second = makeIntrusiveRefCnt<NamespaceTypoProvider>("AAB", "CCC");
+  auto Third = makeIntrusiveRefCnt<NamespaceTypoProvider>("AAB", "DDD");
   DiagnosticWatcher Watcher("AAB", "CCC");
-  Installer->PushSource(&First);
-  Installer->PushSource(&Second);
-  Installer->PushSource(&Third);
+  Installer->PushSource(First.get());
+  Installer->PushSource(Second.get());
+  Installer->PushSource(Third.get());
   Installer->PushWatcher(&Watcher);
   std::vector<std::string> Args(1, "-std=c++11");
   ASSERT_TRUE(clang::tooling::runToolOnCodeWithArgs(
       std::move(Installer), "namespace AAA { } using namespace AAB;", Args));
-  ASSERT_LE(1, First.CallCount);
-  ASSERT_LE(1, Second.CallCount);
-  ASSERT_EQ(0, Third.CallCount);
+  ASSERT_LE(1, First->CallCount);
+  ASSERT_LE(1, Second->CallCount);
+  ASSERT_EQ(0, Third->CallCount);
   ASSERT_EQ(1, Watcher.SeenCount);
 }
 
 TEST(ExternalSemaSource, ExternalDelayedTypoCorrection) {
   auto Installer = std::make_unique<ExternalSemaSourceInstaller>();
-  FunctionTypoProvider Provider("aaa", "bbb");
+  auto Provider = makeIntrusiveRefCnt<FunctionTypoProvider>("aaa", "bbb");
   DiagnosticWatcher Watcher("aaa", "bbb");
-  Installer->PushSource(&Provider);
+  Installer->PushSource(Provider.get());
   Installer->PushWatcher(&Watcher);
   std::vector<std::string> Args(1, "-std=c++11");
   ASSERT_TRUE(clang::tooling::runToolOnCodeWithArgs(
       std::move(Installer), "namespace AAA { } void foo() { AAA::aaa(); }",
       Args));
-  ASSERT_LE(0, Provider.CallCount);
+  ASSERT_LE(0, Provider->CallCount);
   ASSERT_EQ(1, Watcher.SeenCount);
 }
 
@@ -284,8 +286,8 @@ TEST(ExternalSemaSource, ExternalDelayedTypoCorrection) {
 // solve the problem.
 TEST(ExternalSemaSource, TryOtherTacticsBeforeDiagnosing) {
   auto Installer = std::make_unique<ExternalSemaSourceInstaller>();
-  CompleteTypeDiagnoser Diagnoser(false);
-  Installer->PushSource(&Diagnoser);
+  auto Diagnoser = makeIntrusiveRefCnt<CompleteTypeDiagnoser>(false);
+  Installer->PushSource(Diagnoser.get());
   std::vector<std::string> Args(1, "-std=c++11");
   // This code hits the class template specialization/class member of a class
   // template specialization checks in Sema::RequireCompleteTypeImpl.
@@ -293,26 +295,26 @@ TEST(ExternalSemaSource, TryOtherTacticsBeforeDiagnosing) {
       std::move(Installer),
       "template <typename T> struct S { class C { }; }; S<char>::C SCInst;",
       Args));
-  ASSERT_EQ(0, Diagnoser.CallCount);
+  ASSERT_EQ(0, Diagnoser->CallCount);
 }
 
 // The first ExternalSemaSource where MaybeDiagnoseMissingCompleteType returns
 // true should be the last one called.
 TEST(ExternalSemaSource, FirstDiagnoserTaken) {
   auto Installer = std::make_unique<ExternalSemaSourceInstaller>();
-  CompleteTypeDiagnoser First(false);
-  CompleteTypeDiagnoser Second(true);
-  CompleteTypeDiagnoser Third(true);
-  Installer->PushSource(&First);
-  Installer->PushSource(&Second);
-  Installer->PushSource(&Third);
+  auto First = makeIntrusiveRefCnt<CompleteTypeDiagnoser>(false);
+  auto Second = makeIntrusiveRefCnt<CompleteTypeDiagnoser>(true);
+  auto Third = makeIntrusiveRefCnt<CompleteTypeDiagnoser>(true);
+  Installer->PushSource(First.get());
+  Installer->PushSource(Second.get());
+  Installer->PushSource(Third.get());
   std::vector<std::string> Args(1, "-std=c++11");
   ASSERT_FALSE(clang::tooling::runToolOnCodeWithArgs(
       std::move(Installer), "class Incomplete; Incomplete IncompleteInstance;",
       Args));
-  ASSERT_EQ(1, First.CallCount);
-  ASSERT_EQ(1, Second.CallCount);
-  ASSERT_EQ(0, Third.CallCount);
+  ASSERT_EQ(1, First->CallCount);
+  ASSERT_EQ(1, Second->CallCount);
+  ASSERT_EQ(0, Third->CallCount);
 }
 
 } // anonymous namespace