beanz created this revision.
beanz added reviewers: rsmith, aaron.ballman, python3kgae, akyrtzi, aprantl.
Herald added a project: All.
beanz requested review of this revision.
Herald added a project: clang.
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.
Repository:
rG LLVM Github Monorepo
https://reviews.llvm.org/D133158
Files:
clang/include/clang/Sema/MultiplexExternalSemaSource.h
clang/include/clang/Sema/Sema.h
clang/lib/Frontend/ChainedIncludesSource.cpp
clang/lib/Sema/MultiplexExternalSemaSource.cpp
clang/lib/Sema/Sema.cpp
clang/lib/Sema/SemaDeclCXX.cpp
clang/unittests/Sema/ExternalSemaSourceTest.cpp
Index: clang/unittests/Sema/ExternalSemaSourceTest.cpp
===================================================================
--- clang/unittests/Sema/ExternalSemaSourceTest.cpp
+++ clang/unittests/Sema/ExternalSemaSourceTest.cpp
@@ -219,6 +219,8 @@
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 @@
// 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 @@
// 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 @@
// 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 @@
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
Index: clang/lib/Sema/SemaDeclCXX.cpp
===================================================================
--- clang/lib/Sema/SemaDeclCXX.cpp
+++ clang/lib/Sema/SemaDeclCXX.cpp
@@ -18183,7 +18183,7 @@
llvm::SmallPtrSet<CXXConstructorDecl*, 4> Valid, Invalid, Current;
for (DelegatingCtorDeclsType::iterator
- I = DelegatingCtorDecls.begin(ExternalSource),
+ I = DelegatingCtorDecls.begin(ExternalSource.get()),
E = DelegatingCtorDecls.end();
I != E; ++I)
DelegatingCycleHelper(*I, Valid, Invalid, Current, *this);
Index: clang/lib/Sema/Sema.cpp
===================================================================
--- clang/lib/Sema/Sema.cpp
+++ clang/lib/Sema/Sema.cpp
@@ -185,11 +185,10 @@
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 @@
= 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 @@
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,7 +1284,7 @@
// translation unit, with an initializer equal to 0.
llvm::SmallSet<VarDecl *, 32> Seen;
for (TentativeDefinitionsType::iterator
- T = TentativeDefinitions.begin(ExternalSource),
+ T = TentativeDefinitions.begin(ExternalSource.get()),
TEnd = TentativeDefinitions.end();
T != TEnd; ++T) {
VarDecl *VD = (*T)->getActingDefinition();
@@ -1336,7 +1329,7 @@
if (!Diags.hasErrorOccurred() && TUKind != TU_Module) {
// Output warning for unused file scoped decls.
for (UnusedFileScopedDeclsType::iterator
- I = UnusedFileScopedDecls.begin(ExternalSource),
+ I = UnusedFileScopedDecls.begin(ExternalSource.get()),
E = UnusedFileScopedDecls.end(); I != E; ++I) {
if (ShouldRemoveFromUnused(this, *I))
continue;
Index: clang/lib/Sema/MultiplexExternalSemaSource.cpp
===================================================================
--- clang/lib/Sema/MultiplexExternalSemaSource.cpp
+++ clang/lib/Sema/MultiplexExternalSemaSource.cpp
@@ -19,21 +19,27 @@
///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.
///
///\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);
}
//===----------------------------------------------------------------------===//
Index: clang/lib/Frontend/ChainedIncludesSource.cpp
===================================================================
--- clang/lib/Frontend/ChainedIncludesSource.cpp
+++ 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 @@
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 @@
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());
}
Index: clang/include/clang/Sema/Sema.h
===================================================================
--- clang/include/clang/Sema/Sema.h
+++ clang/include/clang/Sema/Sema.h
@@ -360,10 +360,7 @@
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 @@
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);
Index: clang/include/clang/Sema/MultiplexExternalSemaSource.h
===================================================================
--- clang/include/clang/Sema/MultiplexExternalSemaSource.h
+++ clang/include/clang/Sema/MultiplexExternalSemaSource.h
@@ -40,17 +40,17 @@
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
/// 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;
@@ -58,7 +58,7 @@
///
///\param[in] source - An ExternalSemaSource.
///
- void addSource(ExternalSemaSource &source);
+ void AddSource(ExternalSemaSource *Source);
//===--------------------------------------------------------------------===//
// ExternalASTSource.
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
