[PATCH] D133158: [NFC] Make MultiplexExternalSemaSource own sources

[Chris Bieneman via Phabricator via cfe-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rG5b5329bd41ba: [NFC] Make MultiplexExternalSemaSource own 
sources (authored by beanz).

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D133158/new/

https://reviews.llvm.org/D133158

Files:
  clang-tools-extra/clang-include-fixer/IncludeFixer.cpp
  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();
@@ -234,14 +236,14 @@
 // instead of the usual suggestion we would use above.
 TEST(ExternalSemaSource, ExternalTypoCorrectionPrioritized) {
   auto Installer = std::make_unique();
-  NamespaceTypoProvider Provider("AAB", "BBB");
+  auto Provider = makeIntrusiveRefCnt("AAB", "BBB");
   DiagnosticWatcher Watcher("AAB", "BBB");
-  Installer->PushSource(&Provider);
+  Installer->PushSource(Provider.get());
   Installer->PushWatcher(&Watcher);
   std::vector 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();
-  NamespaceTypoProvider First("XXX", "BBB");
-  NamespaceTypoProvider Second("AAB", "CCC");
-  NamespaceTypoProvider Third("AAB", "DDD");
+  auto First = makeIntrusiveRefCnt("XXX", "BBB");
+  auto Second = makeIntrusiveRefCnt("AAB", "CCC");
+  auto Third = makeIntrusiveRefCnt("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 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();
-  FunctionTypoProvider Provider("aaa", "bbb");
+  auto Provider = makeIntrusiveRefCnt("aaa", "bbb");
   DiagnosticWatcher Watcher("aaa", "bbb");
-  Installer->PushSource(&Provider);
+  Installer->PushSource(Provider.get());
   Installer->PushWatcher(&Watcher);
   std::vector 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();
-  CompleteTypeDiagnoser Diagnoser(false);
-  Installer->PushSource(&Diagnoser);
+  auto Diagnoser = makeIntrusiveRefCnt(false);
+  Installer->PushSource(Diagnoser.get());
   std::vector 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  struct S { class C { }; }; S::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();
-  CompleteTypeDiagnoser First(false);
-  CompleteTypeDiagnoser Second(true);
-  CompleteTypeDiagnoser Third(true);
-  Installer->PushSource(&First);
-  Installer->PushSource(&Second);
-  Installer->PushSource(&Third);
+  auto First = makeIntrusiveRefCnt(false);
+  auto Secon

[PATCH] D133158: [NFC] Make MultiplexExternalSemaSource own sources

[Chris Bieneman via Phabricator via cfe-commits]

beanz added a comment.

I built LLDB and ran its tests. I see no additional failures after applying my 
change, but LLDB's tests do not execute successfully on my local system (22 
failures).


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D133158/new/

https://reviews.llvm.org/D133158

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D133158: [NFC] Make MultiplexExternalSemaSource own sources

[Chris Bieneman via Phabricator via cfe-commits]

beanz updated this revision to Diff 457651.
beanz added a comment.
Herald added a project: clang-tools-extra.

Updating with changes based on review feedback, and fixups for clang-tools-extra


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D133158/new/

https://reviews.llvm.org/D133158

Files:
  clang-tools-extra/clang-include-fixer/IncludeFixer.cpp
  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();
@@ -234,14 +236,14 @@
 // instead of the usual suggestion we would use above.
 TEST(ExternalSemaSource, ExternalTypoCorrectionPrioritized) {
   auto Installer = std::make_unique();
-  NamespaceTypoProvider Provider("AAB", "BBB");
+  auto Provider = makeIntrusiveRefCnt("AAB", "BBB");
   DiagnosticWatcher Watcher("AAB", "BBB");
-  Installer->PushSource(&Provider);
+  Installer->PushSource(Provider.get());
   Installer->PushWatcher(&Watcher);
   std::vector 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();
-  NamespaceTypoProvider First("XXX", "BBB");
-  NamespaceTypoProvider Second("AAB", "CCC");
-  NamespaceTypoProvider Third("AAB", "DDD");
+  auto First = makeIntrusiveRefCnt("XXX", "BBB");
+  auto Second = makeIntrusiveRefCnt("AAB", "CCC");
+  auto Third = makeIntrusiveRefCnt("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 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();
-  FunctionTypoProvider Provider("aaa", "bbb");
+  auto Provider = makeIntrusiveRefCnt("aaa", "bbb");
   DiagnosticWatcher Watcher("aaa", "bbb");
-  Installer->PushSource(&Provider);
+  Installer->PushSource(Provider.get());
   Installer->PushWatcher(&Watcher);
   std::vector 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();
-  CompleteTypeDiagnoser Diagnoser(false);
-  Installer->PushSource(&Diagnoser);
+  auto Diagnoser = makeIntrusiveRefCnt(false);
+  Installer->PushSource(Diagnoser.get());
   std::vector 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  struct S { class C { }; }; S::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();
-  CompleteTypeDiagnoser First(false);
-  CompleteTypeDiagnoser Second(true);
-  CompleteTypeDiagnoser Third(true);
-  Installer->PushSource(&First);
-  Installer->PushSource(&Second);
-  Installer->PushSource(&Third);
+  auto First = makeIntrusiveRefCnt(false);
+  auto Second = makeIntrusiveRefCnt(true);
+  auto Thir

[PATCH] D133158: [NFC] Make MultiplexExternalSemaSource own sources

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman accepted this revision.
aaron.ballman added a comment.

LGTM aside from some minor nits (also, please run clang-format over the patch 
before landing).




Comment at: clang/lib/Sema/MultiplexExternalSemaSource.cpp:32
+MultiplexExternalSemaSource::~MultiplexExternalSemaSource() {
+  for (auto &S : Sources)
+S->Release();





Comment at: clang/lib/Sema/Sema.cpp:548
 
-  if (isMultiplexExternalSource)
-static_cast(ExternalSource)->addSource(*E);
-  else {
-ExternalSource = new MultiplexExternalSemaSource(*ExternalSource, *E);
-isMultiplexExternalSource = true;
-  }
+  if (auto Ex = dyn_cast(ExternalSource))
+Ex->AddSource(E);




Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D133158/new/

https://reviews.llvm.org/D133158

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D133158: [NFC] Make MultiplexExternalSemaSource own sources

[Adrian Prantl via Phabricator via cfe-commits]

aprantl accepted this revision.
aprantl added a comment.
This revision is now accepted and ready to land.

This looks plausible to me — did you build clang-tools-extra & lldb to make 
sure nothing else needs to be updated?




Comment at: clang/include/clang/Sema/MultiplexExternalSemaSource.h:53
   ///
-  MultiplexExternalSemaSource(ExternalSemaSource& s1, ExternalSemaSource& s2);
+  MultiplexExternalSemaSource(ExternalSemaSource* S1, ExternalSemaSource* S2);
 

beanz wrote:
> aprantl wrote:
> > why this change? Does `&` imply ownership in our coding style?
> The old implementation took the address of the references to store. It seems 
> more clear to me to accept a pointer when you need a pointer.
> 
> The old interface using references also allowed for passing in 
> stack-allocated objects, which causes problems since the change here calls 
> retain and release on the underlying ref count object. Taking pointers puts 
> the onus on the user of the API to think through where the address passed in 
> comes from.
nit: clang-format :-)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D133158/new/

https://reviews.llvm.org/D133158

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D133158: [NFC] Make MultiplexExternalSemaSource own sources

[Chris Bieneman via Phabricator via cfe-commits]

beanz added inline comments.



Comment at: clang/include/clang/Sema/MultiplexExternalSemaSource.h:53
   ///
-  MultiplexExternalSemaSource(ExternalSemaSource& s1, ExternalSemaSource& s2);
+  MultiplexExternalSemaSource(ExternalSemaSource* S1, ExternalSemaSource* S2);
 

aprantl wrote:
> why this change? Does `&` imply ownership in our coding style?
The old implementation took the address of the references to store. It seems 
more clear to me to accept a pointer when you need a pointer.

The old interface using references also allowed for passing in stack-allocated 
objects, which causes problems since the change here calls retain and release 
on the underlying ref count object. Taking pointers puts the onus on the user 
of the API to think through where the address passed in comes from.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D133158/new/

https://reviews.llvm.org/D133158

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D133158: [NFC] Make MultiplexExternalSemaSource own sources

[Adrian Prantl via Phabricator via cfe-commits]

aprantl added inline comments.



Comment at: clang/include/clang/Sema/MultiplexExternalSemaSource.h:53
   ///
-  MultiplexExternalSemaSource(ExternalSemaSource& s1, ExternalSemaSource& s2);
+  MultiplexExternalSemaSource(ExternalSemaSource* S1, ExternalSemaSource* S2);
 

why this change? Does `&` imply ownership in our coding style?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D133158/new/

https://reviews.llvm.org/D133158

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D133158: [NFC] Make MultiplexExternalSemaSource own sources

[Chris Bieneman via Phabricator via cfe-commits]

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();
@@ -234,14 +236,14 @@
 // instead of the usual suggestion we would use above.
 TEST(ExternalSemaSource, ExternalTypoCorrectionPrioritized) {
   auto Installer = std::make_unique();
-  NamespaceTypoProvider Provider("AAB", "BBB");
+  auto Provider = makeIntrusiveRefCnt("AAB", "BBB");
   DiagnosticWatcher Watcher("AAB", "BBB");
-  Installer->PushSource(&Provider);
+  Installer->PushSource(Provider.get());
   Installer->PushWatcher(&Watcher);
   std::vector 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();
-  NamespaceTypoProvider First("XXX", "BBB");
-  NamespaceTypoProvider Second("AAB", "CCC");
-  NamespaceTypoProvider Third("AAB", "DDD");
+  auto First = makeIntrusiveRefCnt("XXX", "BBB");
+  auto Second = makeIntrusiveRefCnt("AAB", "CCC");
+  auto Third = makeIntrusiveRefCnt("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 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();
-  FunctionTypoProvider Provider("aaa", "bbb");
+  auto Provider = makeIntrusiveRefCnt("aaa", "bbb");
   DiagnosticWatcher Watcher("aaa", "bbb");
-  Installer->PushSource(&Provider);
+  Installer->PushSource(Provider.get());
   Installer->PushWatcher(&Watcher);
   std::vector 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();
-  CompleteTypeDiagnoser Diagnoser(false);
-  Installer->PushSource(&Diagnoser);
+  auto Diagnoser = makeIntrusiveRefCnt(false);
+  Installer->PushSource(Diagnoser.get());
   std::vector 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  struct S { class C { }; }; S::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();
-  CompleteTypeDiagnoser First(false);
-  CompleteTypeDiagnoser Second(true);
-  CompleteTypeDiagnoser Third(true);
-  Installer->PushSource(&First);
-  Installer->PushSource(&Second);
-  Installer->PushSource(&Third);
+