[PATCH] D60815: [clangd] Recognize "don't include me directly" pattern, and suppress include insertion.

[Eric Liu via Phabricator via cfe-commits]

ioeric added inline comments.



Comment at: clangd/index/SymbolCollector.cpp:631
+
+bool SymbolCollector::isSelfContainedHeader(FileID FID) {
+  // The real computation (which will be memoized).

sammccall wrote:
> ioeric wrote:
> > sammccall wrote:
> > > this has been moved to a member so it can use the cache and the 
> > > (non-threadsafe) regex, and its logic has obviously been changed.
> > "self-contained header" doesn't sound accurate anymore. Maybe something 
> > like `isIncludable`?
> Let me explain how I think about it, and then I'll change the name if you 
> still don't think it fits.
> 
> What we're detecting here (usually) is that this header can't be included 
> unless a certain symbol/state is defined first from the outside.
> Whether that's for some direct technical reason (e.g. configuration is 
> needed) or to enforce a coding style, the dependency on external preprocessor 
> state means the header isn't self contained. (And the non-self-containedness 
> is the reason we can't include it in arbitrary contexts) 
That sounds good. Thanks for explaining!


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D60815



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

[PATCH] D60815: [clangd] Recognize "don't include me directly" pattern, and suppress include insertion.

[Eric Liu via Phabricator via cfe-commits]

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

lgtm




Comment at: clangd/index/SymbolCollector.cpp:631
+
+bool SymbolCollector::isSelfContainedHeader(FileID FID) {
+  // The real computation (which will be memoized).

sammccall wrote:
> this has been moved to a member so it can use the cache and the 
> (non-threadsafe) regex, and its logic has obviously been changed.
"self-contained header" doesn't sound accurate anymore. Maybe something like 
`isIncludable`?



Comment at: clangd/index/SymbolCollector.h:125
+  // they need to be included via an umbrella header. e.g. GTK matches this.
+  llvm::Regex DontIncludeMe = {
+  "^[ \t]*#[ \t]*if.*\n" // An #if, #ifndef etc directive, then

`DontIncludeMePattern` or `DontIncludeMeRegex`?


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D60815



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

[PATCH] D60687: [clangd] Check file path of declaring header when deciding whether to insert include.

[Eric Liu via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL358496: [clangd] Check file path of declaring header when 
deciding whether to insert… (authored by ioeric, committed by ).
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.

Repository:
  rL LLVM

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

https://reviews.llvm.org/D60687

Files:
  clang-tools-extra/trunk/clangd/CodeComplete.cpp
  clang-tools-extra/trunk/clangd/Headers.cpp
  clang-tools-extra/trunk/clangd/Headers.h
  clang-tools-extra/trunk/clangd/IncludeFixer.cpp
  clang-tools-extra/trunk/unittests/clangd/HeadersTests.cpp

Index: clang-tools-extra/trunk/unittests/clangd/HeadersTests.cpp
===
--- clang-tools-extra/trunk/unittests/clangd/HeadersTests.cpp
+++ clang-tools-extra/trunk/unittests/clangd/HeadersTests.cpp
@@ -93,11 +93,10 @@
  >getPreprocessor().getHeaderSearchInfo());
 for (const auto  : Inclusions)
   Inserter.addExisting(Inc);
-auto Declaring = ToHeaderFile(Original);
 auto Inserted = ToHeaderFile(Preferred);
-if (!Inserter.shouldInsertInclude(Declaring, Inserted))
+if (!Inserter.shouldInsertInclude(Original, Inserted))
   return "";
-std::string Path = Inserter.calculateIncludePath(Declaring, Inserted);
+std::string Path = Inserter.calculateIncludePath(Inserted);
 Action.EndSourceFile();
 return Path;
   }
@@ -258,16 +257,15 @@
/*BuildDir=*/"", /*HeaderSearchInfo=*/nullptr);
 
   auto HeaderPath = testPath("sub/bar.h");
-  auto Declaring = HeaderFile{HeaderPath, /*Verbatim=*/false};
   auto Inserting = HeaderFile{HeaderPath, /*Verbatim=*/false};
   auto Verbatim = HeaderFile{"", /*Verbatim=*/true};
 
-  EXPECT_EQ(Inserter.calculateIncludePath(Declaring, Inserting),
+  EXPECT_EQ(Inserter.calculateIncludePath(Inserting),
 "\"" + HeaderPath + "\"");
-  EXPECT_EQ(Inserter.shouldInsertInclude(Declaring, Inserting), false);
+  EXPECT_EQ(Inserter.shouldInsertInclude(HeaderPath, Inserting), false);
 
-  EXPECT_EQ(Inserter.calculateIncludePath(Declaring, Verbatim), "");
-  EXPECT_EQ(Inserter.shouldInsertInclude(Declaring, Verbatim), true);
+  EXPECT_EQ(Inserter.calculateIncludePath(Verbatim), "");
+  EXPECT_EQ(Inserter.shouldInsertInclude(HeaderPath, Verbatim), true);
 }
 
 } // namespace
Index: clang-tools-extra/trunk/clangd/Headers.cpp
===
--- clang-tools-extra/trunk/clangd/Headers.cpp
+++ clang-tools-extra/trunk/clangd/Headers.cpp
@@ -173,22 +173,21 @@
 /// FIXME(ioeric): we might not want to insert an absolute include path if the
 /// path is not shortened.
 bool IncludeInserter::shouldInsertInclude(
-const HeaderFile , const HeaderFile ) const {
-  assert(DeclaringHeader.valid() && InsertedHeader.valid());
+PathRef DeclaringHeader, const HeaderFile ) const {
+  assert(InsertedHeader.valid());
   if (!HeaderSearchInfo && !InsertedHeader.Verbatim)
 return false;
-  if (FileName == DeclaringHeader.File || FileName == InsertedHeader.File)
+  if (FileName == DeclaringHeader || FileName == InsertedHeader.File)
 return false;
   auto Included = [&](llvm::StringRef Header) {
 return IncludedHeaders.find(Header) != IncludedHeaders.end();
   };
-  return !Included(DeclaringHeader.File) && !Included(InsertedHeader.File);
+  return !Included(DeclaringHeader) && !Included(InsertedHeader.File);
 }
 
 std::string
-IncludeInserter::calculateIncludePath(const HeaderFile ,
-  const HeaderFile ) const {
-  assert(DeclaringHeader.valid() && InsertedHeader.valid());
+IncludeInserter::calculateIncludePath(const HeaderFile ) const {
+  assert(InsertedHeader.valid());
   if (InsertedHeader.Verbatim)
 return InsertedHeader.File;
   bool IsSystem = false;
Index: clang-tools-extra/trunk/clangd/CodeComplete.cpp
===
--- clang-tools-extra/trunk/clangd/CodeComplete.cpp
+++ clang-tools-extra/trunk/clangd/CodeComplete.cpp
@@ -349,15 +349,18 @@
 // Turn absolute path into a literal string that can be #included.
 auto Inserted = [&](llvm::StringRef Header)
 -> llvm::Expected> {
-  auto ResolvedDeclaring =
-  toHeaderFile(C.IndexResult->CanonicalDeclaration.FileURI, FileName);
+  auto DeclaringURI =
+  URI::parse(C.IndexResult->CanonicalDeclaration.FileURI);
+  if (!DeclaringURI)
+return DeclaringURI.takeError();
+  auto ResolvedDeclaring = URI::resolve(*DeclaringURI, FileName);
   if (!ResolvedDeclaring)
 return ResolvedDeclaring.takeError();
   auto ResolvedInserted = toHeaderFile(Header, FileName);
   if (!ResolvedInserted)
 return ResolvedInserted.takeError();
   return std::make_pair(
-  Includes.calculateIncludePath(*ResolvedDeclaring, 

[PATCH] D60687: [clangd] Check file path of declaring header when deciding whether to insert include.

[Eric Liu via Phabricator via cfe-commits]

ioeric marked an inline comment as done.
ioeric added inline comments.



Comment at: unittests/clangd/HeadersTests.cpp:208
 
 TEST_F(HeadersTest, DoNotInsertIfInSameFile) {
   MainFile = testPath("main.h");

> IIUC, this already fixes the cases we'd seen of include-insertion of a header 
> into itself.
> Is it feasible to add a test case for that?
Here is a test case for it. However, to trigger the old behavior caused by 
include spelling, we would need to implement a custom URI scheme that performs 
include spelling customization. Let me know if you think we should add one.



Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D60687



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

[PATCH] D60607: [clangd] Wait for compile command in ASTWorker instead of ClangdServer

[Eric Liu via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rCTE358400: [clangd] Wait for compile command in ASTWorker 
instead of ClangdServer (authored by ioeric, committed by ).

Changed prior to commit:
  https://reviews.llvm.org/D60607?vs=195139=195141#toc

Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D60607

Files:
  clangd/ClangdServer.cpp
  clangd/ClangdServer.h
  clangd/Compiler.h
  clangd/GlobalCompilationDatabase.cpp
  clangd/TUScheduler.cpp
  clangd/TUScheduler.h
  unittests/clangd/ClangdTests.cpp
  unittests/clangd/CodeCompleteTests.cpp
  unittests/clangd/TUSchedulerTests.cpp

Index: unittests/clangd/ClangdTests.cpp
===
--- unittests/clangd/ClangdTests.cpp
+++ unittests/clangd/ClangdTests.cpp
@@ -1099,6 +1099,64 @@
 Field(::Scope, "ns::";
 }
 
+TEST_F(ClangdVFSTest, FallbackWhenWaitingForCompileCommand) {
+  MockFSProvider FS;
+  ErrorCheckingDiagConsumer DiagConsumer;
+  // Returns compile command only when notified.
+  class DelayedCompilationDatabase : public GlobalCompilationDatabase {
+  public:
+DelayedCompilationDatabase(Notification )
+: CanReturnCommand(CanReturnCommand) {}
+
+llvm::Optional
+getCompileCommand(PathRef File, ProjectInfo * = nullptr) const override {
+  // FIXME: make this timeout and fail instead of waiting forever in case
+  // something goes wrong.
+  CanReturnCommand.wait();
+  auto FileName = llvm::sys::path::filename(File);
+  std::vector CommandLine = {"clangd", "-ffreestanding", File};
+  return {tooling::CompileCommand(llvm::sys::path::parent_path(File),
+  FileName, std::move(CommandLine), "")};
+}
+
+std::vector ExtraClangFlags;
+
+  private:
+Notification 
+  };
+
+  Notification CanReturnCommand;
+  DelayedCompilationDatabase CDB(CanReturnCommand);
+  ClangdServer Server(CDB, FS, DiagConsumer, ClangdServer::optsForTest());
+
+  auto FooCpp = testPath("foo.cpp");
+  Annotations Code(R"cpp(
+namespace ns { int xyz; }
+using namespace ns;
+int main() {
+   xy^
+})cpp");
+  FS.Files[FooCpp] = FooCpp;
+  Server.addDocument(FooCpp, Code.code());
+
+  // Sleep for some time to make sure code completion is not run because update
+  // hasn't been scheduled.
+  std::this_thread::sleep_for(std::chrono::milliseconds(10));
+  auto Opts = clangd::CodeCompleteOptions();
+  Opts.AllowFallback = true;
+
+  auto Res = cantFail(runCodeComplete(Server, FooCpp, Code.point(), Opts));
+  EXPECT_EQ(Res.Context, CodeCompletionContext::CCC_Recovery);
+
+  CanReturnCommand.notify();
+  ASSERT_TRUE(Server.blockUntilIdleForTest());
+  EXPECT_THAT(cantFail(runCodeComplete(Server, FooCpp, Code.point(),
+   clangd::CodeCompleteOptions()))
+  .Completions,
+  ElementsAre(AllOf(Field(::Name, "xyz"),
+Field(::Scope, "ns::";
+}
+
 } // namespace
 } // namespace clangd
 } // namespace clang
Index: unittests/clangd/TUSchedulerTests.cpp
===
--- unittests/clangd/TUSchedulerTests.cpp
+++ unittests/clangd/TUSchedulerTests.cpp
@@ -21,8 +21,6 @@
 namespace clangd {
 namespace {
 
-using ::testing::_;
-using ::testing::AllOf;
 using ::testing::AnyOf;
 using ::testing::Each;
 using ::testing::ElementsAre;
@@ -103,7 +101,7 @@
 TUSchedulerTests::DiagsCallbackKey;
 
 TEST_F(TUSchedulerTests, MissingFiles) {
-  TUScheduler S(getDefaultAsyncThreadsCount(),
+  TUScheduler S(CDB, getDefaultAsyncThreadsCount(),
 /*StorePreamblesInMemory=*/true, /*ASTCallbacks=*/nullptr,
 /*UpdateDebounce=*/std::chrono::steady_clock::duration::zero(),
 ASTRetentionPolicy());
@@ -154,7 +152,7 @@
 // thread until we've scheduled them all.
 Notification Ready;
 TUScheduler S(
-getDefaultAsyncThreadsCount(),
+CDB, getDefaultAsyncThreadsCount(),
 /*StorePreamblesInMemory=*/true, captureDiags(),
 /*UpdateDebounce=*/std::chrono::steady_clock::duration::zero(),
 ASTRetentionPolicy());
@@ -184,7 +182,7 @@
 TEST_F(TUSchedulerTests, Debounce) {
   std::atomic CallbackCount(0);
   {
-TUScheduler S(getDefaultAsyncThreadsCount(),
+TUScheduler S(CDB, getDefaultAsyncThreadsCount(),
   /*StorePreamblesInMemory=*/true, captureDiags(),
   /*UpdateDebounce=*/std::chrono::seconds(1),
   ASTRetentionPolicy());
@@ -220,7 +218,7 @@
   {
 Notification InconsistentReadDone; // Must live longest.
 TUScheduler S(
-getDefaultAsyncThreadsCount(), /*StorePreamblesInMemory=*/true,
+CDB, getDefaultAsyncThreadsCount(), /*StorePreamblesInMemory=*/true,
 /*ASTCallbacks=*/nullptr,
 

[PATCH] D60607: [clangd] Wait for compile command in ASTWorker instead of ClangdServer

[Eric Liu via Phabricator via cfe-commits]

ioeric updated this revision to Diff 195139.
ioeric marked 5 inline comments as done.
ioeric added a comment.

- use sync runAddDocument in test


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D60607

Files:
  clangd/ClangdServer.cpp
  clangd/ClangdServer.h
  clangd/Compiler.h
  clangd/GlobalCompilationDatabase.cpp
  clangd/TUScheduler.cpp
  clangd/TUScheduler.h
  unittests/clangd/ClangdTests.cpp
  unittests/clangd/CodeCompleteTests.cpp
  unittests/clangd/TUSchedulerTests.cpp

Index: unittests/clangd/TUSchedulerTests.cpp
===
--- unittests/clangd/TUSchedulerTests.cpp
+++ unittests/clangd/TUSchedulerTests.cpp
@@ -21,8 +21,6 @@
 namespace clangd {
 namespace {
 
-using ::testing::_;
-using ::testing::AllOf;
 using ::testing::AnyOf;
 using ::testing::Each;
 using ::testing::ElementsAre;
@@ -103,7 +101,7 @@
 TUSchedulerTests::DiagsCallbackKey;
 
 TEST_F(TUSchedulerTests, MissingFiles) {
-  TUScheduler S(getDefaultAsyncThreadsCount(),
+  TUScheduler S(CDB, getDefaultAsyncThreadsCount(),
 /*StorePreamblesInMemory=*/true, /*ASTCallbacks=*/nullptr,
 /*UpdateDebounce=*/std::chrono::steady_clock::duration::zero(),
 ASTRetentionPolicy());
@@ -154,7 +152,7 @@
 // thread until we've scheduled them all.
 Notification Ready;
 TUScheduler S(
-getDefaultAsyncThreadsCount(),
+CDB, getDefaultAsyncThreadsCount(),
 /*StorePreamblesInMemory=*/true, captureDiags(),
 /*UpdateDebounce=*/std::chrono::steady_clock::duration::zero(),
 ASTRetentionPolicy());
@@ -184,7 +182,7 @@
 TEST_F(TUSchedulerTests, Debounce) {
   std::atomic CallbackCount(0);
   {
-TUScheduler S(getDefaultAsyncThreadsCount(),
+TUScheduler S(CDB, getDefaultAsyncThreadsCount(),
   /*StorePreamblesInMemory=*/true, captureDiags(),
   /*UpdateDebounce=*/std::chrono::seconds(1),
   ASTRetentionPolicy());
@@ -220,7 +218,7 @@
   {
 Notification InconsistentReadDone; // Must live longest.
 TUScheduler S(
-getDefaultAsyncThreadsCount(), /*StorePreamblesInMemory=*/true,
+CDB, getDefaultAsyncThreadsCount(), /*StorePreamblesInMemory=*/true,
 /*ASTCallbacks=*/nullptr,
 /*UpdateDebounce=*/std::chrono::steady_clock::duration::zero(),
 ASTRetentionPolicy());
@@ -277,7 +275,7 @@
   {
 Notification Proceed; // Ensure we schedule everything.
 TUScheduler S(
-getDefaultAsyncThreadsCount(), /*StorePreamblesInMemory=*/true,
+CDB, getDefaultAsyncThreadsCount(), /*StorePreamblesInMemory=*/true,
 /*ASTCallbacks=*/captureDiags(),
 /*UpdateDebounce=*/std::chrono::steady_clock::duration::zero(),
 ASTRetentionPolicy());
@@ -346,7 +344,7 @@
 
   // Run TUScheduler and collect some stats.
   {
-TUScheduler S(getDefaultAsyncThreadsCount(),
+TUScheduler S(CDB, getDefaultAsyncThreadsCount(),
   /*StorePreamblesInMemory=*/true, captureDiags(),
   /*UpdateDebounce=*/std::chrono::milliseconds(50),
   ASTRetentionPolicy());
@@ -437,10 +435,11 @@
   std::atomic BuiltASTCounter(0);
   ASTRetentionPolicy Policy;
   Policy.MaxRetainedASTs = 2;
-  TUScheduler S(
-  /*AsyncThreadsCount=*/1, /*StorePreambleInMemory=*/true,
-  /*ASTCallbacks=*/nullptr,
-  /*UpdateDebounce=*/std::chrono::steady_clock::duration::zero(), Policy);
+  TUScheduler S(CDB,
+/*AsyncThreadsCount=*/1, /*StorePreambleInMemory=*/true,
+/*ASTCallbacks=*/nullptr,
+/*UpdateDebounce=*/std::chrono::steady_clock::duration::zero(),
+Policy);
 
   llvm::StringLiteral SourceContents = R"cpp(
 int* a;
@@ -487,11 +486,11 @@
 }
 
 TEST_F(TUSchedulerTests, EmptyPreamble) {
-  TUScheduler S(
-  /*AsyncThreadsCount=*/4, /*StorePreambleInMemory=*/true,
-  /*ASTCallbacks=*/nullptr,
-  /*UpdateDebounce=*/std::chrono::steady_clock::duration::zero(),
-  ASTRetentionPolicy());
+  TUScheduler S(CDB,
+/*AsyncThreadsCount=*/4, /*StorePreambleInMemory=*/true,
+/*ASTCallbacks=*/nullptr,
+/*UpdateDebounce=*/std::chrono::steady_clock::duration::zero(),
+ASTRetentionPolicy());
 
   auto Foo = testPath("foo.cpp");
   auto Header = testPath("foo.h");
@@ -532,11 +531,11 @@
 TEST_F(TUSchedulerTests, RunWaitsForPreamble) {
   // Testing strategy: we update the file and schedule a few preamble reads at
   // the same time. All reads should get the same non-null preamble.
-  TUScheduler S(
-  /*AsyncThreadsCount=*/4, /*StorePreambleInMemory=*/true,
-  /*ASTCallbacks=*/nullptr,
-  /*UpdateDebounce=*/std::chrono::steady_clock::duration::zero(),
-  ASTRetentionPolicy());
+  TUScheduler S(CDB,
+/*AsyncThreadsCount=*/4, 

[PATCH] D60607: [clangd] Wait for compile command in ASTWorker instead of ClangdServer

[Eric Liu via Phabricator via cfe-commits]

ioeric updated this revision to Diff 195137.
ioeric marked 9 inline comments as done.
ioeric added a comment.

- address comments


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D60607

Files:
  clangd/ClangdServer.cpp
  clangd/ClangdServer.h
  clangd/Compiler.h
  clangd/GlobalCompilationDatabase.cpp
  clangd/TUScheduler.cpp
  clangd/TUScheduler.h
  unittests/clangd/ClangdTests.cpp
  unittests/clangd/CodeCompleteTests.cpp
  unittests/clangd/TUSchedulerTests.cpp

Index: unittests/clangd/TUSchedulerTests.cpp
===
--- unittests/clangd/TUSchedulerTests.cpp
+++ unittests/clangd/TUSchedulerTests.cpp
@@ -21,8 +21,6 @@
 namespace clangd {
 namespace {
 
-using ::testing::_;
-using ::testing::AllOf;
 using ::testing::AnyOf;
 using ::testing::Each;
 using ::testing::ElementsAre;
@@ -103,7 +101,7 @@
 TUSchedulerTests::DiagsCallbackKey;
 
 TEST_F(TUSchedulerTests, MissingFiles) {
-  TUScheduler S(getDefaultAsyncThreadsCount(),
+  TUScheduler S(CDB, getDefaultAsyncThreadsCount(),
 /*StorePreamblesInMemory=*/true, /*ASTCallbacks=*/nullptr,
 /*UpdateDebounce=*/std::chrono::steady_clock::duration::zero(),
 ASTRetentionPolicy());
@@ -154,7 +152,7 @@
 // thread until we've scheduled them all.
 Notification Ready;
 TUScheduler S(
-getDefaultAsyncThreadsCount(),
+CDB, getDefaultAsyncThreadsCount(),
 /*StorePreamblesInMemory=*/true, captureDiags(),
 /*UpdateDebounce=*/std::chrono::steady_clock::duration::zero(),
 ASTRetentionPolicy());
@@ -184,7 +182,7 @@
 TEST_F(TUSchedulerTests, Debounce) {
   std::atomic CallbackCount(0);
   {
-TUScheduler S(getDefaultAsyncThreadsCount(),
+TUScheduler S(CDB, getDefaultAsyncThreadsCount(),
   /*StorePreamblesInMemory=*/true, captureDiags(),
   /*UpdateDebounce=*/std::chrono::seconds(1),
   ASTRetentionPolicy());
@@ -220,7 +218,7 @@
   {
 Notification InconsistentReadDone; // Must live longest.
 TUScheduler S(
-getDefaultAsyncThreadsCount(), /*StorePreamblesInMemory=*/true,
+CDB, getDefaultAsyncThreadsCount(), /*StorePreamblesInMemory=*/true,
 /*ASTCallbacks=*/nullptr,
 /*UpdateDebounce=*/std::chrono::steady_clock::duration::zero(),
 ASTRetentionPolicy());
@@ -277,7 +275,7 @@
   {
 Notification Proceed; // Ensure we schedule everything.
 TUScheduler S(
-getDefaultAsyncThreadsCount(), /*StorePreamblesInMemory=*/true,
+CDB, getDefaultAsyncThreadsCount(), /*StorePreamblesInMemory=*/true,
 /*ASTCallbacks=*/captureDiags(),
 /*UpdateDebounce=*/std::chrono::steady_clock::duration::zero(),
 ASTRetentionPolicy());
@@ -346,7 +344,7 @@
 
   // Run TUScheduler and collect some stats.
   {
-TUScheduler S(getDefaultAsyncThreadsCount(),
+TUScheduler S(CDB, getDefaultAsyncThreadsCount(),
   /*StorePreamblesInMemory=*/true, captureDiags(),
   /*UpdateDebounce=*/std::chrono::milliseconds(50),
   ASTRetentionPolicy());
@@ -437,10 +435,11 @@
   std::atomic BuiltASTCounter(0);
   ASTRetentionPolicy Policy;
   Policy.MaxRetainedASTs = 2;
-  TUScheduler S(
-  /*AsyncThreadsCount=*/1, /*StorePreambleInMemory=*/true,
-  /*ASTCallbacks=*/nullptr,
-  /*UpdateDebounce=*/std::chrono::steady_clock::duration::zero(), Policy);
+  TUScheduler S(CDB,
+/*AsyncThreadsCount=*/1, /*StorePreambleInMemory=*/true,
+/*ASTCallbacks=*/nullptr,
+/*UpdateDebounce=*/std::chrono::steady_clock::duration::zero(),
+Policy);
 
   llvm::StringLiteral SourceContents = R"cpp(
 int* a;
@@ -487,11 +486,11 @@
 }
 
 TEST_F(TUSchedulerTests, EmptyPreamble) {
-  TUScheduler S(
-  /*AsyncThreadsCount=*/4, /*StorePreambleInMemory=*/true,
-  /*ASTCallbacks=*/nullptr,
-  /*UpdateDebounce=*/std::chrono::steady_clock::duration::zero(),
-  ASTRetentionPolicy());
+  TUScheduler S(CDB,
+/*AsyncThreadsCount=*/4, /*StorePreambleInMemory=*/true,
+/*ASTCallbacks=*/nullptr,
+/*UpdateDebounce=*/std::chrono::steady_clock::duration::zero(),
+ASTRetentionPolicy());
 
   auto Foo = testPath("foo.cpp");
   auto Header = testPath("foo.h");
@@ -532,11 +531,11 @@
 TEST_F(TUSchedulerTests, RunWaitsForPreamble) {
   // Testing strategy: we update the file and schedule a few preamble reads at
   // the same time. All reads should get the same non-null preamble.
-  TUScheduler S(
-  /*AsyncThreadsCount=*/4, /*StorePreambleInMemory=*/true,
-  /*ASTCallbacks=*/nullptr,
-  /*UpdateDebounce=*/std::chrono::steady_clock::duration::zero(),
-  ASTRetentionPolicy());
+  TUScheduler S(CDB,
+/*AsyncThreadsCount=*/4, /*StorePreambleInMemory=*/true,
+   

[PATCH] D60607: [clangd] Wait for compile command in ASTWorker instead of ClangdServer

[Eric Liu via Phabricator via cfe-commits]

ioeric added inline comments.



Comment at: unittests/clangd/ClangdTests.cpp:1148
+  EXPECT_EQ(Res.Context, CodeCompletionContext::CCC_Recovery);
+  // Identifier-based fallback completion doesn't know about "symbol" scope.
+  EXPECT_THAT(Res.Completions,

ilya-biryukov wrote:
> Not strictly related to this patch, but maybe we could add a flag to 
> completion results to indicate if the completion happened via a fallback mode 
> or not?
> 
> Would make the test code more straightforward and the tests like these would 
> not rely on a particular implementation of the fallback mode (e.g. I can 
> imagine the fallback mode learning about the scopes later on)
We are setting the context to `Recovery` and make fallback as part of 
`Recovery`. Do you we should distinguish fallback mode from `Recovery`?



Comment at: unittests/clangd/CodeCompleteTests.cpp:1390
   Server.addDocument(FooCpp, Source.code(), WantDiagnostics::Yes);
+  ASSERT_TRUE(Server.blockUntilIdleForTest());
   CodeCompleteResult Completions = cantFail(runCodeComplete(

ilya-biryukov wrote:
> Could you expand why we need this?
Added a comment.


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D60607



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

[PATCH] D60687: [clangd] Check file path of declaring header when deciding whether to insert include.

[Eric Liu via Phabricator via cfe-commits]

ioeric created this revision.
ioeric added a reviewer: sammccall.
Herald added subscribers: cfe-commits, kadircet, arphaman, jkorous, MaskRay, 
ilya-biryukov.
Herald added a project: clang.

Previously, we would use include spelling of the declaring header to check
whether the inserted header is the same as the main file. This doesn't help 
because
we only have file path of the main file.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D60687

Files:
  clangd/CodeComplete.cpp
  clangd/Headers.cpp
  clangd/Headers.h
  clangd/IncludeFixer.cpp
  unittests/clangd/HeadersTests.cpp

Index: unittests/clangd/HeadersTests.cpp
===
--- unittests/clangd/HeadersTests.cpp
+++ unittests/clangd/HeadersTests.cpp
@@ -93,11 +93,10 @@
  >getPreprocessor().getHeaderSearchInfo());
 for (const auto  : Inclusions)
   Inserter.addExisting(Inc);
-auto Declaring = ToHeaderFile(Original);
 auto Inserted = ToHeaderFile(Preferred);
-if (!Inserter.shouldInsertInclude(Declaring, Inserted))
+if (!Inserter.shouldInsertInclude(Original, Inserted))
   return "";
-std::string Path = Inserter.calculateIncludePath(Declaring, Inserted);
+std::string Path = Inserter.calculateIncludePath(Inserted);
 Action.EndSourceFile();
 return Path;
   }
@@ -258,16 +257,15 @@
/*BuildDir=*/"", /*HeaderSearchInfo=*/nullptr);
 
   auto HeaderPath = testPath("sub/bar.h");
-  auto Declaring = HeaderFile{HeaderPath, /*Verbatim=*/false};
   auto Inserting = HeaderFile{HeaderPath, /*Verbatim=*/false};
   auto Verbatim = HeaderFile{"", /*Verbatim=*/true};
 
-  EXPECT_EQ(Inserter.calculateIncludePath(Declaring, Inserting),
+  EXPECT_EQ(Inserter.calculateIncludePath(Inserting),
 "\"" + HeaderPath + "\"");
-  EXPECT_EQ(Inserter.shouldInsertInclude(Declaring, Inserting), false);
+  EXPECT_EQ(Inserter.shouldInsertInclude(HeaderPath, Inserting), false);
 
-  EXPECT_EQ(Inserter.calculateIncludePath(Declaring, Verbatim), "");
-  EXPECT_EQ(Inserter.shouldInsertInclude(Declaring, Verbatim), true);
+  EXPECT_EQ(Inserter.calculateIncludePath(Verbatim), "");
+  EXPECT_EQ(Inserter.shouldInsertInclude(HeaderPath, Verbatim), true);
 }
 
 } // namespace
Index: clangd/IncludeFixer.cpp
===
--- clangd/IncludeFixer.cpp
+++ clangd/IncludeFixer.cpp
@@ -142,15 +142,17 @@
 std::vector IncludeFixer::fixesForSymbols(const SymbolSlab ) const {
   auto Inserted = [&](const Symbol , llvm::StringRef Header)
   -> llvm::Expected> {
-auto ResolvedDeclaring =
-toHeaderFile(Sym.CanonicalDeclaration.FileURI, File);
+auto DeclaringURI = URI::parse(Sym.CanonicalDeclaration.FileURI);
+if (!DeclaringURI)
+  return DeclaringURI.takeError();
+auto ResolvedDeclaring = URI::resolve(*DeclaringURI, File);
 if (!ResolvedDeclaring)
   return ResolvedDeclaring.takeError();
 auto ResolvedInserted = toHeaderFile(Header, File);
 if (!ResolvedInserted)
   return ResolvedInserted.takeError();
 return std::make_pair(
-Inserter->calculateIncludePath(*ResolvedDeclaring, *ResolvedInserted),
+Inserter->calculateIncludePath(*ResolvedInserted),
 Inserter->shouldInsertInclude(*ResolvedDeclaring, *ResolvedInserted));
   };
 
@@ -173,8 +175,8 @@
 {std::move(*Edit)}});
 }
   } else {
-vlog("Failed to calculate include insertion for {0} into {1}: {2}",
- File, Inc, ToInclude.takeError());
+vlog("Failed to calculate include insertion for {0} into {1}: {2}", Inc,
+ File, ToInclude.takeError());
   }
 }
   }
Index: clangd/Headers.h
===
--- clangd/Headers.h
+++ clangd/Headers.h
@@ -137,25 +137,22 @@
   ///   in \p Inclusions (including those included via different paths).
   ///   - \p DeclaringHeader or \p InsertedHeader is the same as \p File.
   ///
-  /// \param DeclaringHeader is the original header corresponding to \p
+  /// \param DeclaringHeader is path of the original header corresponding to \p
   /// InsertedHeader e.g. the header that declares a symbol.
   /// \param InsertedHeader The preferred header to be inserted. This could be
   /// the same as DeclaringHeader but must be provided.
-  bool shouldInsertInclude(const HeaderFile ,
+  bool shouldInsertInclude(PathRef DeclaringHeader,
const HeaderFile ) const;
 
   /// Determines the preferred way to #include a file, taking into account the
   /// search path. Usually this will prefer a shorter representation like
   /// 'Foo/Bar.h' over a longer one like 'Baz/include/Foo/Bar.h'.
   ///
-  /// \param DeclaringHeader is the original header corresponding to \p
-  /// InsertedHeader e.g. the header that declares a symbol.
   /// \param InsertedHeader The preferred header to be 

[PATCH] D60607: [clangd] Wait for compile command in ASTWorker instead of ClangdServer

[Eric Liu via Phabricator via cfe-commits]

ioeric updated this revision to Diff 195111.
ioeric added a comment.

- Only return compile command (instead of FileInputs) from AST worker.


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D60607

Files:
  clangd/ClangdServer.cpp
  clangd/ClangdServer.h
  clangd/Compiler.h
  clangd/GlobalCompilationDatabase.cpp
  clangd/TUScheduler.cpp
  clangd/TUScheduler.h
  unittests/clangd/ClangdTests.cpp
  unittests/clangd/CodeCompleteTests.cpp
  unittests/clangd/TUSchedulerTests.cpp

Index: unittests/clangd/TUSchedulerTests.cpp
===
--- unittests/clangd/TUSchedulerTests.cpp
+++ unittests/clangd/TUSchedulerTests.cpp
@@ -21,8 +21,6 @@
 namespace clangd {
 namespace {
 
-using ::testing::_;
-using ::testing::AllOf;
 using ::testing::AnyOf;
 using ::testing::Each;
 using ::testing::ElementsAre;
@@ -103,7 +101,7 @@
 TUSchedulerTests::DiagsCallbackKey;
 
 TEST_F(TUSchedulerTests, MissingFiles) {
-  TUScheduler S(getDefaultAsyncThreadsCount(),
+  TUScheduler S(CDB, getDefaultAsyncThreadsCount(),
 /*StorePreamblesInMemory=*/true, /*ASTCallbacks=*/nullptr,
 /*UpdateDebounce=*/std::chrono::steady_clock::duration::zero(),
 ASTRetentionPolicy());
@@ -154,7 +152,7 @@
 // thread until we've scheduled them all.
 Notification Ready;
 TUScheduler S(
-getDefaultAsyncThreadsCount(),
+CDB, getDefaultAsyncThreadsCount(),
 /*StorePreamblesInMemory=*/true, captureDiags(),
 /*UpdateDebounce=*/std::chrono::steady_clock::duration::zero(),
 ASTRetentionPolicy());
@@ -184,7 +182,7 @@
 TEST_F(TUSchedulerTests, Debounce) {
   std::atomic CallbackCount(0);
   {
-TUScheduler S(getDefaultAsyncThreadsCount(),
+TUScheduler S(CDB, getDefaultAsyncThreadsCount(),
   /*StorePreamblesInMemory=*/true, captureDiags(),
   /*UpdateDebounce=*/std::chrono::seconds(1),
   ASTRetentionPolicy());
@@ -220,7 +218,7 @@
   {
 Notification InconsistentReadDone; // Must live longest.
 TUScheduler S(
-getDefaultAsyncThreadsCount(), /*StorePreamblesInMemory=*/true,
+CDB, getDefaultAsyncThreadsCount(), /*StorePreamblesInMemory=*/true,
 /*ASTCallbacks=*/nullptr,
 /*UpdateDebounce=*/std::chrono::steady_clock::duration::zero(),
 ASTRetentionPolicy());
@@ -277,7 +275,7 @@
   {
 Notification Proceed; // Ensure we schedule everything.
 TUScheduler S(
-getDefaultAsyncThreadsCount(), /*StorePreamblesInMemory=*/true,
+CDB, getDefaultAsyncThreadsCount(), /*StorePreamblesInMemory=*/true,
 /*ASTCallbacks=*/captureDiags(),
 /*UpdateDebounce=*/std::chrono::steady_clock::duration::zero(),
 ASTRetentionPolicy());
@@ -346,7 +344,7 @@
 
   // Run TUScheduler and collect some stats.
   {
-TUScheduler S(getDefaultAsyncThreadsCount(),
+TUScheduler S(CDB, getDefaultAsyncThreadsCount(),
   /*StorePreamblesInMemory=*/true, captureDiags(),
   /*UpdateDebounce=*/std::chrono::milliseconds(50),
   ASTRetentionPolicy());
@@ -437,10 +435,11 @@
   std::atomic BuiltASTCounter(0);
   ASTRetentionPolicy Policy;
   Policy.MaxRetainedASTs = 2;
-  TUScheduler S(
-  /*AsyncThreadsCount=*/1, /*StorePreambleInMemory=*/true,
-  /*ASTCallbacks=*/nullptr,
-  /*UpdateDebounce=*/std::chrono::steady_clock::duration::zero(), Policy);
+  TUScheduler S(CDB,
+/*AsyncThreadsCount=*/1, /*StorePreambleInMemory=*/true,
+/*ASTCallbacks=*/nullptr,
+/*UpdateDebounce=*/std::chrono::steady_clock::duration::zero(),
+Policy);
 
   llvm::StringLiteral SourceContents = R"cpp(
 int* a;
@@ -487,11 +486,11 @@
 }
 
 TEST_F(TUSchedulerTests, EmptyPreamble) {
-  TUScheduler S(
-  /*AsyncThreadsCount=*/4, /*StorePreambleInMemory=*/true,
-  /*ASTCallbacks=*/nullptr,
-  /*UpdateDebounce=*/std::chrono::steady_clock::duration::zero(),
-  ASTRetentionPolicy());
+  TUScheduler S(CDB,
+/*AsyncThreadsCount=*/4, /*StorePreambleInMemory=*/true,
+/*ASTCallbacks=*/nullptr,
+/*UpdateDebounce=*/std::chrono::steady_clock::duration::zero(),
+ASTRetentionPolicy());
 
   auto Foo = testPath("foo.cpp");
   auto Header = testPath("foo.h");
@@ -532,11 +531,11 @@
 TEST_F(TUSchedulerTests, RunWaitsForPreamble) {
   // Testing strategy: we update the file and schedule a few preamble reads at
   // the same time. All reads should get the same non-null preamble.
-  TUScheduler S(
-  /*AsyncThreadsCount=*/4, /*StorePreambleInMemory=*/true,
-  /*ASTCallbacks=*/nullptr,
-  /*UpdateDebounce=*/std::chrono::steady_clock::duration::zero(),
-  ASTRetentionPolicy());
+  TUScheduler S(CDB,
+/*AsyncThreadsCount=*/4, 

[PATCH] D60257: [Lookup] Invisible decls should not be ambiguous when renaming.

[Eric Liu via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rC358378: [Lookup] Invisible decls should not be ambiguous 
when renaming. (authored by ioeric, committed by ).

Changed prior to commit:
  https://reviews.llvm.org/D60257?vs=194841=195103#toc

Repository:
  rC Clang

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

https://reviews.llvm.org/D60257

Files:
  include/clang/Tooling/Core/Lookup.h
  lib/Tooling/Core/Lookup.cpp
  lib/Tooling/Refactoring/Rename/USRLocFinder.cpp
  unittests/Tooling/LookupTest.cpp

Index: lib/Tooling/Refactoring/Rename/USRLocFinder.cpp
===
--- lib/Tooling/Refactoring/Rename/USRLocFinder.cpp
+++ lib/Tooling/Refactoring/Rename/USRLocFinder.cpp
@@ -542,8 +542,8 @@
 if (!llvm::isa(
 RenameInfo.Context->getDeclContext())) {
   ReplacedName = tooling::replaceNestedName(
-  RenameInfo.Specifier, RenameInfo.Context->getDeclContext(),
-  RenameInfo.FromDecl,
+  RenameInfo.Specifier, RenameInfo.Begin,
+  RenameInfo.Context->getDeclContext(), RenameInfo.FromDecl,
   NewName.startswith("::") ? NewName.str()
: ("::" + NewName).str());
 } else {
Index: lib/Tooling/Core/Lookup.cpp
===
--- lib/Tooling/Core/Lookup.cpp
+++ lib/Tooling/Core/Lookup.cpp
@@ -14,6 +14,7 @@
 #include "clang/AST/Decl.h"
 #include "clang/AST/DeclCXX.h"
 #include "clang/AST/DeclarationName.h"
+#include "clang/Basic/SourceLocation.h"
 #include "llvm/ADT/SmallVector.h"
 using namespace clang;
 using namespace clang::tooling;
@@ -123,7 +124,8 @@
 // FIXME: consider using namespaces.
 static std::string disambiguateSpellingInScope(StringRef Spelling,
StringRef QName,
-   const DeclContext ) {
+   const DeclContext ,
+   SourceLocation UseLoc) {
   assert(QName.startswith("::"));
   assert(QName.endswith(Spelling));
   if (Spelling.startswith("::"))
@@ -138,9 +140,10 @@
   getAllNamedNamespaces();
   auto  = UseContext.getParentASTContext();
   StringRef TrimmedQName = QName.substr(2);
+  const auto  = UseContext.getParentASTContext().getSourceManager();
+  UseLoc = SM.getSpellingLoc(UseLoc);
 
-  auto IsAmbiguousSpelling = [, , ](
- const llvm::StringRef CurSpelling) {
+  auto IsAmbiguousSpelling = [&](const llvm::StringRef CurSpelling) {
 if (CurSpelling.startswith("::"))
   return false;
 // Lookup the first component of Spelling in all enclosing namespaces
@@ -151,7 +154,13 @@
   auto LookupRes = NS->lookup(DeclarationName((Head)));
   if (!LookupRes.empty()) {
 for (const NamedDecl *Res : LookupRes)
-  if (!TrimmedQName.startswith(Res->getQualifiedNameAsString()))
+  // If `Res` is not visible in `UseLoc`, we don't consider it
+  // ambiguous. For example, a reference in a header file should not be
+  // affected by a potentially ambiguous name in some file that includes
+  // the header.
+  if (!TrimmedQName.startswith(Res->getQualifiedNameAsString()) &&
+  SM.isBeforeInTranslationUnit(
+  SM.getSpellingLoc(Res->getLocation()), UseLoc))
 return true;
   }
 }
@@ -172,6 +181,7 @@
 }
 
 std::string tooling::replaceNestedName(const NestedNameSpecifier *Use,
+   SourceLocation UseLoc,
const DeclContext *UseContext,
const NamedDecl *FromDecl,
StringRef ReplacementString) {
@@ -206,5 +216,6 @@
   StringRef Suggested = getBestNamespaceSubstr(UseContext, ReplacementString,
isFullyQualified(Use));
 
-  return disambiguateSpellingInScope(Suggested, ReplacementString, *UseContext);
+  return disambiguateSpellingInScope(Suggested, ReplacementString, *UseContext,
+ UseLoc);
 }
Index: unittests/Tooling/LookupTest.cpp
===
--- unittests/Tooling/LookupTest.cpp
+++ unittests/Tooling/LookupTest.cpp
@@ -44,8 +44,8 @@
 const auto *Callee = cast(Expr->getCallee()->IgnoreImplicit());
 const ValueDecl *FD = Callee->getDecl();
 return tooling::replaceNestedName(
-Callee->getQualifier(), Visitor.DeclStack.back()->getDeclContext(), FD,
-ReplacementString);
+Callee->getQualifier(), Callee->getLocation(),
+Visitor.DeclStack.back()->getDeclContext(), FD, ReplacementString);
   };
 
   Visitor.OnCall = [&](CallExpr *Expr) {
@@ -181,12 +181,12 @@
 

[PATCH] D60607: [clangd] Wait for compile command in ASTWorker instead of ClangdServer

[Eric Liu via Phabricator via cfe-commits]

ioeric marked an inline comment as done.
ioeric added inline comments.



Comment at: clangd/TUScheduler.cpp:552
+const tooling::CompileCommand ::getCurrentCompileCommand() const {
+  return FileInputs.CompileCommand;
+}

ioeric wrote:
> ilya-biryukov wrote:
> > ioeric wrote:
> > > ilya-biryukov wrote:
> > > > ioeric wrote:
> > > > > ilya-biryukov wrote:
> > > > > > ioeric wrote:
> > > > > > > ilya-biryukov wrote:
> > > > > > > > This is racy, `FileInputs` is only accessed from a worker 
> > > > > > > > thread now.
> > > > > > > > I'm afraid we'll need a separate variable with a lock around it 
> > > > > > > > (could reuse one lock for preamble and compile command, 
> > > > > > > > probably)
> > > > > > > It looks like we could also guard `FileInputs`? Adding another 
> > > > > > > variable seems to make the state more complicated. 
> > > > > > `FileInputs` are currently accessed without locks in a bunch of 
> > > > > > places and it'd be nice to keep it that way (the alternative is 
> > > > > > more complicated code).
> > > > > > Doing the same thing we do for preamble looks like the best 
> > > > > > alternative here.
> > > > > The reason I think it would be easier to guard `FileInputs` with 
> > > > > mutex instead of pulling a new variable is that CompileCommand is 
> > > > > usually used along with other fields in `FileInputs`.  I think we 
> > > > > could manage this with relatively few changes. Please take a look at 
> > > > > the new changes.
> > > > Unfortunately we can't share `Inputs.FS` safely as the vfs 
> > > > implementations are not thread-safe.
> > > It seems to me that the behavior for `FS` hasn't change in this patch. 
> > > And `FileInputs` (incl. `Inputs.FS`) has always been available/shared 
> > > across worker threads. We don't seem to have systematic way to prevent 
> > > raciness before this patch, and it would be good to have it somehow, but 
> > > it's probably out of the scope.  
> > > 
> > > Maybe I'm missing something or misinterpreting. Could you elaborate?  
> > > And FileInputs (incl. Inputs.FS) has always been available/shared across 
> > > worker threads
> > I don't think that's the case, we did not a public API to get the hands on 
> > `ASTWorker::FileInputs.FS` and we're getting one now.
> > Even though the current patch does not add such racy accesses, it certainly 
> > makes it much easier to do it accidentally from the clients of `ASTWorker` 
> > in the future (one just needs to access `getCurrentInputs().FS`).
> > 
> > The (not very) systematic way to prevent raciness that we use now is to 
> > **not** protect the members which are potentially racy with locks and 
> > document they are accessed only from a worker thread.
> > Having a separate copy of the compile command is a small price to pay (both 
> > in terms of performance and complexity) to avoid exposing non-thread-safe 
> > members of `ASTWorker` in its public interface.
> I don't think that makes a fundamental difference as `FileInputs.FS` is 
> already racy. But if "public" API is the concern, we could simply restrict 
> the scope of the API and make return `CompileCommand` only.
Inside `ASTWorker`, `FileInputs` can still be guarded with `Mutex` as a whole. 


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D60607



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

[PATCH] D60607: [clangd] Wait for compile command in ASTWorker instead of ClangdServer

[Eric Liu via Phabricator via cfe-commits]

ioeric marked an inline comment as done.
ioeric added inline comments.



Comment at: clangd/TUScheduler.cpp:552
+const tooling::CompileCommand ::getCurrentCompileCommand() const {
+  return FileInputs.CompileCommand;
+}

ilya-biryukov wrote:
> ioeric wrote:
> > ilya-biryukov wrote:
> > > ioeric wrote:
> > > > ilya-biryukov wrote:
> > > > > ioeric wrote:
> > > > > > ilya-biryukov wrote:
> > > > > > > This is racy, `FileInputs` is only accessed from a worker thread 
> > > > > > > now.
> > > > > > > I'm afraid we'll need a separate variable with a lock around it 
> > > > > > > (could reuse one lock for preamble and compile command, probably)
> > > > > > It looks like we could also guard `FileInputs`? Adding another 
> > > > > > variable seems to make the state more complicated. 
> > > > > `FileInputs` are currently accessed without locks in a bunch of 
> > > > > places and it'd be nice to keep it that way (the alternative is more 
> > > > > complicated code).
> > > > > Doing the same thing we do for preamble looks like the best 
> > > > > alternative here.
> > > > The reason I think it would be easier to guard `FileInputs` with mutex 
> > > > instead of pulling a new variable is that CompileCommand is usually 
> > > > used along with other fields in `FileInputs`.  I think we could manage 
> > > > this with relatively few changes. Please take a look at the new changes.
> > > Unfortunately we can't share `Inputs.FS` safely as the vfs 
> > > implementations are not thread-safe.
> > It seems to me that the behavior for `FS` hasn't change in this patch. And 
> > `FileInputs` (incl. `Inputs.FS`) has always been available/shared across 
> > worker threads. We don't seem to have systematic way to prevent raciness 
> > before this patch, and it would be good to have it somehow, but it's 
> > probably out of the scope.  
> > 
> > Maybe I'm missing something or misinterpreting. Could you elaborate?  
> > And FileInputs (incl. Inputs.FS) has always been available/shared across 
> > worker threads
> I don't think that's the case, we did not a public API to get the hands on 
> `ASTWorker::FileInputs.FS` and we're getting one now.
> Even though the current patch does not add such racy accesses, it certainly 
> makes it much easier to do it accidentally from the clients of `ASTWorker` in 
> the future (one just needs to access `getCurrentInputs().FS`).
> 
> The (not very) systematic way to prevent raciness that we use now is to 
> **not** protect the members which are potentially racy with locks and 
> document they are accessed only from a worker thread.
> Having a separate copy of the compile command is a small price to pay (both 
> in terms of performance and complexity) to avoid exposing non-thread-safe 
> members of `ASTWorker` in its public interface.
I don't think that makes a fundamental difference as `FileInputs.FS` is already 
racy. But if "public" API is the concern, we could simply restrict the scope of 
the API and make return `CompileCommand` only.


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D60607



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

[PATCH] D60607: [clangd] Wait for compile command in ASTWorker instead of ClangdServer

[Eric Liu via Phabricator via cfe-commits]

ioeric marked an inline comment as done.
ioeric added inline comments.



Comment at: clangd/TUScheduler.cpp:552
+const tooling::CompileCommand ::getCurrentCompileCommand() const {
+  return FileInputs.CompileCommand;
+}

ilya-biryukov wrote:
> ioeric wrote:
> > ilya-biryukov wrote:
> > > ioeric wrote:
> > > > ilya-biryukov wrote:
> > > > > This is racy, `FileInputs` is only accessed from a worker thread now.
> > > > > I'm afraid we'll need a separate variable with a lock around it 
> > > > > (could reuse one lock for preamble and compile command, probably)
> > > > It looks like we could also guard `FileInputs`? Adding another variable 
> > > > seems to make the state more complicated. 
> > > `FileInputs` are currently accessed without locks in a bunch of places 
> > > and it'd be nice to keep it that way (the alternative is more complicated 
> > > code).
> > > Doing the same thing we do for preamble looks like the best alternative 
> > > here.
> > The reason I think it would be easier to guard `FileInputs` with mutex 
> > instead of pulling a new variable is that CompileCommand is usually used 
> > along with other fields in `FileInputs`.  I think we could manage this with 
> > relatively few changes. Please take a look at the new changes.
> Unfortunately we can't share `Inputs.FS` safely as the vfs implementations 
> are not thread-safe.
It seems to me that the behavior for `FS` hasn't change in this patch. And 
`FileInputs` (incl. `Inputs.FS`) has always been available/shared across worker 
threads. We don't seem to have systematic way to prevent raciness before this 
patch, and it would be good to have it somehow, but it's probably out of the 
scope.  

Maybe I'm missing something or misinterpreting. Could you elaborate?  


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D60607



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

[PATCH] D60607: [clangd] Wait for compile command in ASTWorker instead of ClangdServer

[Eric Liu via Phabricator via cfe-commits]

ioeric added inline comments.



Comment at: clangd/TUScheduler.cpp:338
+  Barrier(Barrier), Done(false) {
+  FileInputs.CompileCommand = CDB.getFallbackCommand(FileName);
+}

ilya-biryukov wrote:
> ioeric wrote:
> > ilya-biryukov wrote:
> > > The command is filled with a fallback by `ClangdServer`, right? Why do we 
> > > need to repeat this here?
> > `ASTWorker::FileInputs` is not set until ASTWorker runs update and gets 
> > compile command. Before that, `FileInputs` contains empty compile command 
> > that can be used by `runWithPreamble`/`runWithAST` (afaict). Initializing 
> > it to fallback command seems to be a better alternative.
> That makes sense. Could we remove the `getFallbackCommand` call from the 
> `ClangdServer::addDocument`?
> Let's make the `TUScheduler` fully responsible for the compile command.
Sounds good. 



Comment at: clangd/TUScheduler.cpp:552
+const tooling::CompileCommand ::getCurrentCompileCommand() const {
+  return FileInputs.CompileCommand;
+}

ilya-biryukov wrote:
> ioeric wrote:
> > ilya-biryukov wrote:
> > > This is racy, `FileInputs` is only accessed from a worker thread now.
> > > I'm afraid we'll need a separate variable with a lock around it (could 
> > > reuse one lock for preamble and compile command, probably)
> > It looks like we could also guard `FileInputs`? Adding another variable 
> > seems to make the state more complicated. 
> `FileInputs` are currently accessed without locks in a bunch of places and 
> it'd be nice to keep it that way (the alternative is more complicated code).
> Doing the same thing we do for preamble looks like the best alternative here.
The reason I think it would be easier to guard `FileInputs` with mutex instead 
of pulling a new variable is that CompileCommand is usually used along with 
other fields in `FileInputs`.  I think we could manage this with relatively few 
changes. Please take a look at the new changes.


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D60607



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

[PATCH] D60607: [clangd] Wait for compile command in ASTWorker instead of ClangdServer

[Eric Liu via Phabricator via cfe-commits]

ioeric updated this revision to Diff 194891.
ioeric marked 5 inline comments as done.
ioeric added a comment.

- address comments


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D60607

Files:
  clangd/ClangdServer.cpp
  clangd/ClangdServer.h
  clangd/Compiler.h
  clangd/GlobalCompilationDatabase.cpp
  clangd/TUScheduler.cpp
  clangd/TUScheduler.h
  unittests/clangd/ClangdTests.cpp
  unittests/clangd/CodeCompleteTests.cpp
  unittests/clangd/TUSchedulerTests.cpp

Index: unittests/clangd/TUSchedulerTests.cpp
===
--- unittests/clangd/TUSchedulerTests.cpp
+++ unittests/clangd/TUSchedulerTests.cpp
@@ -21,8 +21,6 @@
 namespace clangd {
 namespace {
 
-using ::testing::_;
-using ::testing::AllOf;
 using ::testing::AnyOf;
 using ::testing::Each;
 using ::testing::ElementsAre;
@@ -103,7 +101,7 @@
 TUSchedulerTests::DiagsCallbackKey;
 
 TEST_F(TUSchedulerTests, MissingFiles) {
-  TUScheduler S(getDefaultAsyncThreadsCount(),
+  TUScheduler S(CDB, getDefaultAsyncThreadsCount(),
 /*StorePreamblesInMemory=*/true, /*ASTCallbacks=*/nullptr,
 /*UpdateDebounce=*/std::chrono::steady_clock::duration::zero(),
 ASTRetentionPolicy());
@@ -154,7 +152,7 @@
 // thread until we've scheduled them all.
 Notification Ready;
 TUScheduler S(
-getDefaultAsyncThreadsCount(),
+CDB, getDefaultAsyncThreadsCount(),
 /*StorePreamblesInMemory=*/true, captureDiags(),
 /*UpdateDebounce=*/std::chrono::steady_clock::duration::zero(),
 ASTRetentionPolicy());
@@ -184,7 +182,7 @@
 TEST_F(TUSchedulerTests, Debounce) {
   std::atomic CallbackCount(0);
   {
-TUScheduler S(getDefaultAsyncThreadsCount(),
+TUScheduler S(CDB, getDefaultAsyncThreadsCount(),
   /*StorePreamblesInMemory=*/true, captureDiags(),
   /*UpdateDebounce=*/std::chrono::seconds(1),
   ASTRetentionPolicy());
@@ -220,7 +218,7 @@
   {
 Notification InconsistentReadDone; // Must live longest.
 TUScheduler S(
-getDefaultAsyncThreadsCount(), /*StorePreamblesInMemory=*/true,
+CDB, getDefaultAsyncThreadsCount(), /*StorePreamblesInMemory=*/true,
 /*ASTCallbacks=*/nullptr,
 /*UpdateDebounce=*/std::chrono::steady_clock::duration::zero(),
 ASTRetentionPolicy());
@@ -277,7 +275,7 @@
   {
 Notification Proceed; // Ensure we schedule everything.
 TUScheduler S(
-getDefaultAsyncThreadsCount(), /*StorePreamblesInMemory=*/true,
+CDB, getDefaultAsyncThreadsCount(), /*StorePreamblesInMemory=*/true,
 /*ASTCallbacks=*/captureDiags(),
 /*UpdateDebounce=*/std::chrono::steady_clock::duration::zero(),
 ASTRetentionPolicy());
@@ -346,7 +344,7 @@
 
   // Run TUScheduler and collect some stats.
   {
-TUScheduler S(getDefaultAsyncThreadsCount(),
+TUScheduler S(CDB, getDefaultAsyncThreadsCount(),
   /*StorePreamblesInMemory=*/true, captureDiags(),
   /*UpdateDebounce=*/std::chrono::milliseconds(50),
   ASTRetentionPolicy());
@@ -437,10 +435,11 @@
   std::atomic BuiltASTCounter(0);
   ASTRetentionPolicy Policy;
   Policy.MaxRetainedASTs = 2;
-  TUScheduler S(
-  /*AsyncThreadsCount=*/1, /*StorePreambleInMemory=*/true,
-  /*ASTCallbacks=*/nullptr,
-  /*UpdateDebounce=*/std::chrono::steady_clock::duration::zero(), Policy);
+  TUScheduler S(CDB,
+/*AsyncThreadsCount=*/1, /*StorePreambleInMemory=*/true,
+/*ASTCallbacks=*/nullptr,
+/*UpdateDebounce=*/std::chrono::steady_clock::duration::zero(),
+Policy);
 
   llvm::StringLiteral SourceContents = R"cpp(
 int* a;
@@ -487,11 +486,11 @@
 }
 
 TEST_F(TUSchedulerTests, EmptyPreamble) {
-  TUScheduler S(
-  /*AsyncThreadsCount=*/4, /*StorePreambleInMemory=*/true,
-  /*ASTCallbacks=*/nullptr,
-  /*UpdateDebounce=*/std::chrono::steady_clock::duration::zero(),
-  ASTRetentionPolicy());
+  TUScheduler S(CDB,
+/*AsyncThreadsCount=*/4, /*StorePreambleInMemory=*/true,
+/*ASTCallbacks=*/nullptr,
+/*UpdateDebounce=*/std::chrono::steady_clock::duration::zero(),
+ASTRetentionPolicy());
 
   auto Foo = testPath("foo.cpp");
   auto Header = testPath("foo.h");
@@ -532,11 +531,11 @@
 TEST_F(TUSchedulerTests, RunWaitsForPreamble) {
   // Testing strategy: we update the file and schedule a few preamble reads at
   // the same time. All reads should get the same non-null preamble.
-  TUScheduler S(
-  /*AsyncThreadsCount=*/4, /*StorePreambleInMemory=*/true,
-  /*ASTCallbacks=*/nullptr,
-  /*UpdateDebounce=*/std::chrono::steady_clock::duration::zero(),
-  ASTRetentionPolicy());
+  TUScheduler S(CDB,
+/*AsyncThreadsCount=*/4, /*StorePreambleInMemory=*/true,
+   

[PATCH] D60607: [clangd] Wait for compile command in ASTWorker instead of ClangdServer

[Eric Liu via Phabricator via cfe-commits]

ioeric added inline comments.



Comment at: clangd/TUScheduler.cpp:338
+  Barrier(Barrier), Done(false) {
+  FileInputs.CompileCommand = CDB.getFallbackCommand(FileName);
+}

ilya-biryukov wrote:
> The command is filled with a fallback by `ClangdServer`, right? Why do we 
> need to repeat this here?
`ASTWorker::FileInputs` is not set until ASTWorker runs update and gets compile 
command. Before that, `FileInputs` contains empty compile command that can be 
used by `runWithPreamble`/`runWithAST` (afaict). Initializing it to fallback 
command seems to be a better alternative.



Comment at: clangd/TUScheduler.cpp:359
+if (auto Cmd = CDB.getCompileCommand(FileName))
+  Inputs.CompileCommand = *Cmd;
 // Will be used to check if we can avoid rebuilding the AST.

ilya-biryukov wrote:
> Maybe update the heuristic field in the current compile command if its empty 
> to indicate we're using an older command?
> That might happen if a previous call to `getCompileCommand` succeeded (so the 
> stored command does not have a heuristic set).
If `CDB.getCompileCommand` failed, we would use `Inputs.CompileCommand` which 
should be the fallback command set in ClangdServer. It might be a good idea to 
use the old command, but it doesn't seem to be in the scope of this patch. 
Added a `FIXME`.



Comment at: clangd/TUScheduler.cpp:552
+const tooling::CompileCommand ::getCurrentCompileCommand() const {
+  return FileInputs.CompileCommand;
+}

ilya-biryukov wrote:
> This is racy, `FileInputs` is only accessed from a worker thread now.
> I'm afraid we'll need a separate variable with a lock around it (could reuse 
> one lock for preamble and compile command, probably)
It looks like we could also guard `FileInputs`? Adding another variable seems 
to make the state more complicated. 



Comment at: clangd/TUScheduler.cpp:835
 
 void TUScheduler::update(PathRef File, ParseInputs Inputs,
  WantDiagnostics WantDiags) {

ilya-biryukov wrote:
> We should add a comment that compile command in inputs is ignored.
> IMO even better is to accept an `FS` and `Contents` instead of `ParseInputs`.
Added commen. Unfortunately, `ParseInputs` contains other things like `Index` 
and `Opts`, so we couldn't replace it with FS and Contents.


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D60607



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

[PATCH] D60607: [clangd] Wait for compile command in ASTWorker instead of ClangdServer

[Eric Liu via Phabricator via cfe-commits]

ioeric updated this revision to Diff 194850.
ioeric marked 7 inline comments as done.
ioeric added a comment.

- address review comments


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D60607

Files:
  clangd/ClangdServer.cpp
  clangd/ClangdServer.h
  clangd/Compiler.h
  clangd/GlobalCompilationDatabase.cpp
  clangd/TUScheduler.cpp
  clangd/TUScheduler.h
  unittests/clangd/ClangdTests.cpp
  unittests/clangd/CodeCompleteTests.cpp
  unittests/clangd/TUSchedulerTests.cpp

Index: unittests/clangd/TUSchedulerTests.cpp
===
--- unittests/clangd/TUSchedulerTests.cpp
+++ unittests/clangd/TUSchedulerTests.cpp
@@ -21,8 +21,6 @@
 namespace clangd {
 namespace {
 
-using ::testing::_;
-using ::testing::AllOf;
 using ::testing::AnyOf;
 using ::testing::Each;
 using ::testing::ElementsAre;
@@ -103,7 +101,7 @@
 TUSchedulerTests::DiagsCallbackKey;
 
 TEST_F(TUSchedulerTests, MissingFiles) {
-  TUScheduler S(getDefaultAsyncThreadsCount(),
+  TUScheduler S(CDB, getDefaultAsyncThreadsCount(),
 /*StorePreamblesInMemory=*/true, /*ASTCallbacks=*/nullptr,
 /*UpdateDebounce=*/std::chrono::steady_clock::duration::zero(),
 ASTRetentionPolicy());
@@ -154,7 +152,7 @@
 // thread until we've scheduled them all.
 Notification Ready;
 TUScheduler S(
-getDefaultAsyncThreadsCount(),
+CDB, getDefaultAsyncThreadsCount(),
 /*StorePreamblesInMemory=*/true, captureDiags(),
 /*UpdateDebounce=*/std::chrono::steady_clock::duration::zero(),
 ASTRetentionPolicy());
@@ -184,7 +182,7 @@
 TEST_F(TUSchedulerTests, Debounce) {
   std::atomic CallbackCount(0);
   {
-TUScheduler S(getDefaultAsyncThreadsCount(),
+TUScheduler S(CDB, getDefaultAsyncThreadsCount(),
   /*StorePreamblesInMemory=*/true, captureDiags(),
   /*UpdateDebounce=*/std::chrono::seconds(1),
   ASTRetentionPolicy());
@@ -220,7 +218,7 @@
   {
 Notification InconsistentReadDone; // Must live longest.
 TUScheduler S(
-getDefaultAsyncThreadsCount(), /*StorePreamblesInMemory=*/true,
+CDB, getDefaultAsyncThreadsCount(), /*StorePreamblesInMemory=*/true,
 /*ASTCallbacks=*/nullptr,
 /*UpdateDebounce=*/std::chrono::steady_clock::duration::zero(),
 ASTRetentionPolicy());
@@ -277,7 +275,7 @@
   {
 Notification Proceed; // Ensure we schedule everything.
 TUScheduler S(
-getDefaultAsyncThreadsCount(), /*StorePreamblesInMemory=*/true,
+CDB, getDefaultAsyncThreadsCount(), /*StorePreamblesInMemory=*/true,
 /*ASTCallbacks=*/captureDiags(),
 /*UpdateDebounce=*/std::chrono::steady_clock::duration::zero(),
 ASTRetentionPolicy());
@@ -346,7 +344,7 @@
 
   // Run TUScheduler and collect some stats.
   {
-TUScheduler S(getDefaultAsyncThreadsCount(),
+TUScheduler S(CDB, getDefaultAsyncThreadsCount(),
   /*StorePreamblesInMemory=*/true, captureDiags(),
   /*UpdateDebounce=*/std::chrono::milliseconds(50),
   ASTRetentionPolicy());
@@ -437,10 +435,11 @@
   std::atomic BuiltASTCounter(0);
   ASTRetentionPolicy Policy;
   Policy.MaxRetainedASTs = 2;
-  TUScheduler S(
-  /*AsyncThreadsCount=*/1, /*StorePreambleInMemory=*/true,
-  /*ASTCallbacks=*/nullptr,
-  /*UpdateDebounce=*/std::chrono::steady_clock::duration::zero(), Policy);
+  TUScheduler S(CDB,
+/*AsyncThreadsCount=*/1, /*StorePreambleInMemory=*/true,
+/*ASTCallbacks=*/nullptr,
+/*UpdateDebounce=*/std::chrono::steady_clock::duration::zero(),
+Policy);
 
   llvm::StringLiteral SourceContents = R"cpp(
 int* a;
@@ -487,11 +486,11 @@
 }
 
 TEST_F(TUSchedulerTests, EmptyPreamble) {
-  TUScheduler S(
-  /*AsyncThreadsCount=*/4, /*StorePreambleInMemory=*/true,
-  /*ASTCallbacks=*/nullptr,
-  /*UpdateDebounce=*/std::chrono::steady_clock::duration::zero(),
-  ASTRetentionPolicy());
+  TUScheduler S(CDB,
+/*AsyncThreadsCount=*/4, /*StorePreambleInMemory=*/true,
+/*ASTCallbacks=*/nullptr,
+/*UpdateDebounce=*/std::chrono::steady_clock::duration::zero(),
+ASTRetentionPolicy());
 
   auto Foo = testPath("foo.cpp");
   auto Header = testPath("foo.h");
@@ -532,11 +531,11 @@
 TEST_F(TUSchedulerTests, RunWaitsForPreamble) {
   // Testing strategy: we update the file and schedule a few preamble reads at
   // the same time. All reads should get the same non-null preamble.
-  TUScheduler S(
-  /*AsyncThreadsCount=*/4, /*StorePreambleInMemory=*/true,
-  /*ASTCallbacks=*/nullptr,
-  /*UpdateDebounce=*/std::chrono::steady_clock::duration::zero(),
-  ASTRetentionPolicy());
+  TUScheduler S(CDB,
+/*AsyncThreadsCount=*/4, 

[PATCH] D60607: [clangd] Wait for compile command in ASTWorker instead of ClangdServer

[Eric Liu via Phabricator via cfe-commits]

ioeric updated this revision to Diff 194851.
ioeric added a comment.

- Add missing comment to TUScheduler.h


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D60607

Files:
  clangd/ClangdServer.cpp
  clangd/ClangdServer.h
  clangd/Compiler.h
  clangd/GlobalCompilationDatabase.cpp
  clangd/TUScheduler.cpp
  clangd/TUScheduler.h
  unittests/clangd/ClangdTests.cpp
  unittests/clangd/CodeCompleteTests.cpp
  unittests/clangd/TUSchedulerTests.cpp

Index: unittests/clangd/TUSchedulerTests.cpp
===
--- unittests/clangd/TUSchedulerTests.cpp
+++ unittests/clangd/TUSchedulerTests.cpp
@@ -21,8 +21,6 @@
 namespace clangd {
 namespace {
 
-using ::testing::_;
-using ::testing::AllOf;
 using ::testing::AnyOf;
 using ::testing::Each;
 using ::testing::ElementsAre;
@@ -103,7 +101,7 @@
 TUSchedulerTests::DiagsCallbackKey;
 
 TEST_F(TUSchedulerTests, MissingFiles) {
-  TUScheduler S(getDefaultAsyncThreadsCount(),
+  TUScheduler S(CDB, getDefaultAsyncThreadsCount(),
 /*StorePreamblesInMemory=*/true, /*ASTCallbacks=*/nullptr,
 /*UpdateDebounce=*/std::chrono::steady_clock::duration::zero(),
 ASTRetentionPolicy());
@@ -154,7 +152,7 @@
 // thread until we've scheduled them all.
 Notification Ready;
 TUScheduler S(
-getDefaultAsyncThreadsCount(),
+CDB, getDefaultAsyncThreadsCount(),
 /*StorePreamblesInMemory=*/true, captureDiags(),
 /*UpdateDebounce=*/std::chrono::steady_clock::duration::zero(),
 ASTRetentionPolicy());
@@ -184,7 +182,7 @@
 TEST_F(TUSchedulerTests, Debounce) {
   std::atomic CallbackCount(0);
   {
-TUScheduler S(getDefaultAsyncThreadsCount(),
+TUScheduler S(CDB, getDefaultAsyncThreadsCount(),
   /*StorePreamblesInMemory=*/true, captureDiags(),
   /*UpdateDebounce=*/std::chrono::seconds(1),
   ASTRetentionPolicy());
@@ -220,7 +218,7 @@
   {
 Notification InconsistentReadDone; // Must live longest.
 TUScheduler S(
-getDefaultAsyncThreadsCount(), /*StorePreamblesInMemory=*/true,
+CDB, getDefaultAsyncThreadsCount(), /*StorePreamblesInMemory=*/true,
 /*ASTCallbacks=*/nullptr,
 /*UpdateDebounce=*/std::chrono::steady_clock::duration::zero(),
 ASTRetentionPolicy());
@@ -277,7 +275,7 @@
   {
 Notification Proceed; // Ensure we schedule everything.
 TUScheduler S(
-getDefaultAsyncThreadsCount(), /*StorePreamblesInMemory=*/true,
+CDB, getDefaultAsyncThreadsCount(), /*StorePreamblesInMemory=*/true,
 /*ASTCallbacks=*/captureDiags(),
 /*UpdateDebounce=*/std::chrono::steady_clock::duration::zero(),
 ASTRetentionPolicy());
@@ -346,7 +344,7 @@
 
   // Run TUScheduler and collect some stats.
   {
-TUScheduler S(getDefaultAsyncThreadsCount(),
+TUScheduler S(CDB, getDefaultAsyncThreadsCount(),
   /*StorePreamblesInMemory=*/true, captureDiags(),
   /*UpdateDebounce=*/std::chrono::milliseconds(50),
   ASTRetentionPolicy());
@@ -437,10 +435,11 @@
   std::atomic BuiltASTCounter(0);
   ASTRetentionPolicy Policy;
   Policy.MaxRetainedASTs = 2;
-  TUScheduler S(
-  /*AsyncThreadsCount=*/1, /*StorePreambleInMemory=*/true,
-  /*ASTCallbacks=*/nullptr,
-  /*UpdateDebounce=*/std::chrono::steady_clock::duration::zero(), Policy);
+  TUScheduler S(CDB,
+/*AsyncThreadsCount=*/1, /*StorePreambleInMemory=*/true,
+/*ASTCallbacks=*/nullptr,
+/*UpdateDebounce=*/std::chrono::steady_clock::duration::zero(),
+Policy);
 
   llvm::StringLiteral SourceContents = R"cpp(
 int* a;
@@ -487,11 +486,11 @@
 }
 
 TEST_F(TUSchedulerTests, EmptyPreamble) {
-  TUScheduler S(
-  /*AsyncThreadsCount=*/4, /*StorePreambleInMemory=*/true,
-  /*ASTCallbacks=*/nullptr,
-  /*UpdateDebounce=*/std::chrono::steady_clock::duration::zero(),
-  ASTRetentionPolicy());
+  TUScheduler S(CDB,
+/*AsyncThreadsCount=*/4, /*StorePreambleInMemory=*/true,
+/*ASTCallbacks=*/nullptr,
+/*UpdateDebounce=*/std::chrono::steady_clock::duration::zero(),
+ASTRetentionPolicy());
 
   auto Foo = testPath("foo.cpp");
   auto Header = testPath("foo.h");
@@ -532,11 +531,11 @@
 TEST_F(TUSchedulerTests, RunWaitsForPreamble) {
   // Testing strategy: we update the file and schedule a few preamble reads at
   // the same time. All reads should get the same non-null preamble.
-  TUScheduler S(
-  /*AsyncThreadsCount=*/4, /*StorePreambleInMemory=*/true,
-  /*ASTCallbacks=*/nullptr,
-  /*UpdateDebounce=*/std::chrono::steady_clock::duration::zero(),
-  ASTRetentionPolicy());
+  TUScheduler S(CDB,
+/*AsyncThreadsCount=*/4, /*StorePreambleInMemory=*/true,
+

[PATCH] D60257: [Lookup] Invisible decls should not be ambiguous when renaming.

[Eric Liu via Phabricator via cfe-commits]

ioeric updated this revision to Diff 194841.
ioeric marked 2 inline comments as done.
ioeric added a comment.

- Add test comment.


Repository:
  rC Clang

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

https://reviews.llvm.org/D60257

Files:
  include/clang/Tooling/Core/Lookup.h
  lib/Tooling/Core/Lookup.cpp
  lib/Tooling/Refactoring/Rename/USRLocFinder.cpp
  unittests/Tooling/LookupTest.cpp

Index: unittests/Tooling/LookupTest.cpp
===
--- unittests/Tooling/LookupTest.cpp
+++ unittests/Tooling/LookupTest.cpp
@@ -44,8 +44,8 @@
 const auto *Callee = cast(Expr->getCallee()->IgnoreImplicit());
 const ValueDecl *FD = Callee->getDecl();
 return tooling::replaceNestedName(
-Callee->getQualifier(), Visitor.DeclStack.back()->getDeclContext(), FD,
-ReplacementString);
+Callee->getQualifier(), Callee->getLocation(),
+Visitor.DeclStack.back()->getDeclContext(), FD, ReplacementString);
   };
 
   Visitor.OnCall = [&](CallExpr *Expr) {
@@ -181,12 +181,12 @@
 TEST(LookupTest, replaceNestedClassName) {
   GetDeclsVisitor Visitor;
 
-  auto replaceRecordTypeLoc = [&](RecordTypeLoc Loc,
+  auto replaceRecordTypeLoc = [&](RecordTypeLoc TLoc,
   StringRef ReplacementString) {
-const auto *FD = cast(Loc.getDecl());
+const auto *FD = cast(TLoc.getDecl());
 return tooling::replaceNestedName(
-nullptr, Visitor.DeclStack.back()->getDeclContext(), FD,
-ReplacementString);
+nullptr, TLoc.getBeginLoc(), Visitor.DeclStack.back()->getDeclContext(),
+FD, ReplacementString);
   };
 
   Visitor.OnRecordTypeLoc = [&](RecordTypeLoc Type) {
@@ -211,6 +211,40 @@
   };
   Visitor.runOver("namespace a { namespace b { class Foo {}; } }\n"
   "namespace c { using a::b::Foo; Foo f();; }\n");
+
+  // Rename TypeLoc `x::y::Old` to new name `x::Foo` at [0] and check that the
+  // type is replaced with "Foo" instead of "x::Foo". Although there is a symbol
+  // `x::y::Foo` in c.cc [1], it should not make "Foo" at [0] ambiguous because
+  // it's not visible at [0].
+  Visitor.OnRecordTypeLoc = [&](RecordTypeLoc Type) {
+if (Type.getDecl()->getQualifiedNameAsString() == "x::y::Old")
+  EXPECT_EQ("Foo", replaceRecordTypeLoc(Type, "::x::Foo"));
+  };
+  Visitor.runOver(R"(
+// a.h
+namespace x {
+ namespace y {
+  class Old {};
+  class Other {};
+ }
+}
+
+// b.h
+namespace x {
+ namespace y {
+  // This is to be renamed to x::Foo
+  // The expected replacement is "Foo".
+  Old f;  // [0].
+ }
+}
+
+// c.cc
+namespace x {
+namespace y {
+ using Foo = ::x::y::Other; // [1]
+}
+}
+)");
 }
 
 } // end anonymous namespace
Index: lib/Tooling/Refactoring/Rename/USRLocFinder.cpp
===
--- lib/Tooling/Refactoring/Rename/USRLocFinder.cpp
+++ lib/Tooling/Refactoring/Rename/USRLocFinder.cpp
@@ -542,8 +542,8 @@
 if (!llvm::isa(
 RenameInfo.Context->getDeclContext())) {
   ReplacedName = tooling::replaceNestedName(
-  RenameInfo.Specifier, RenameInfo.Context->getDeclContext(),
-  RenameInfo.FromDecl,
+  RenameInfo.Specifier, RenameInfo.Begin,
+  RenameInfo.Context->getDeclContext(), RenameInfo.FromDecl,
   NewName.startswith("::") ? NewName.str()
: ("::" + NewName).str());
 } else {
Index: lib/Tooling/Core/Lookup.cpp
===
--- lib/Tooling/Core/Lookup.cpp
+++ lib/Tooling/Core/Lookup.cpp
@@ -14,6 +14,7 @@
 #include "clang/AST/Decl.h"
 #include "clang/AST/DeclCXX.h"
 #include "clang/AST/DeclarationName.h"
+#include "clang/Basic/SourceLocation.h"
 #include "llvm/ADT/SmallVector.h"
 using namespace clang;
 using namespace clang::tooling;
@@ -123,7 +124,8 @@
 // FIXME: consider using namespaces.
 static std::string disambiguateSpellingInScope(StringRef Spelling,
StringRef QName,
-   const DeclContext ) {
+   const DeclContext ,
+   SourceLocation UseLoc) {
   assert(QName.startswith("::"));
   assert(QName.endswith(Spelling));
   if (Spelling.startswith("::"))
@@ -138,9 +140,10 @@
   getAllNamedNamespaces();
   auto  = UseContext.getParentASTContext();
   StringRef TrimmedQName = QName.substr(2);
+  const auto  = UseContext.getParentASTContext().getSourceManager();
+  UseLoc = SM.getSpellingLoc(UseLoc);
 
-  auto IsAmbiguousSpelling = [, , ](
- const llvm::StringRef CurSpelling) {
+  auto IsAmbiguousSpelling = [&](const llvm::StringRef CurSpelling) {
 if (CurSpelling.startswith("::"))
 

[PATCH] D60257: [Lookup] Invisible decls should not be ambiguous when renaming.

[Eric Liu via Phabricator via cfe-commits]

ioeric added inline comments.



Comment at: unittests/Tooling/LookupTest.cpp:215
+
+  // Potentially ambiguous symbols that are not visible at reference location
+  // are not considered ambiguous.

hokein wrote:
> The test seems hard to understand what it actually does, could you explain 
> it? which symbol is ambiguous here? What's the behavior of the test before vs 
> after the fix?
added comment. PTAL


Repository:
  rC Clang

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

https://reviews.llvm.org/D60257



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

[PATCH] D60316: [clangd] Include insertion: require header guards, drop other heuristics, treat .def like .inc.

[Eric Liu via Phabricator via cfe-commits]

ioeric requested changes to this revision.
ioeric added a comment.
This revision now requires changes to proceed.

in case you missed this patch :)


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D60316



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

[PATCH] D60607: [clangd] Wait for compile command in ASTWorker instead of ClangdServer

[Eric Liu via Phabricator via cfe-commits]

ioeric created this revision.
ioeric added reviewers: sammccall, ilya-biryukov.
Herald added subscribers: cfe-commits, kadircet, arphaman, jkorous, MaskRay, 
javed.absar.
Herald added a project: clang.

This makes addDocument non-blocking and would also allow code completion
(in fallback mode) to run when worker waits for the compile command.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D60607

Files:
  clangd/ClangdServer.cpp
  clangd/ClangdServer.h
  clangd/Compiler.h
  clangd/GlobalCompilationDatabase.cpp
  clangd/TUScheduler.cpp
  clangd/TUScheduler.h
  unittests/clangd/ClangdTests.cpp
  unittests/clangd/CodeCompleteTests.cpp
  unittests/clangd/TUSchedulerTests.cpp

Index: unittests/clangd/TUSchedulerTests.cpp
===
--- unittests/clangd/TUSchedulerTests.cpp
+++ unittests/clangd/TUSchedulerTests.cpp
@@ -21,8 +21,6 @@
 namespace clangd {
 namespace {
 
-using ::testing::_;
-using ::testing::AllOf;
 using ::testing::AnyOf;
 using ::testing::Each;
 using ::testing::ElementsAre;
@@ -103,7 +101,7 @@
 TUSchedulerTests::DiagsCallbackKey;
 
 TEST_F(TUSchedulerTests, MissingFiles) {
-  TUScheduler S(getDefaultAsyncThreadsCount(),
+  TUScheduler S(CDB, getDefaultAsyncThreadsCount(),
 /*StorePreamblesInMemory=*/true, /*ASTCallbacks=*/nullptr,
 /*UpdateDebounce=*/std::chrono::steady_clock::duration::zero(),
 ASTRetentionPolicy());
@@ -154,7 +152,7 @@
 // thread until we've scheduled them all.
 Notification Ready;
 TUScheduler S(
-getDefaultAsyncThreadsCount(),
+CDB, getDefaultAsyncThreadsCount(),
 /*StorePreamblesInMemory=*/true, captureDiags(),
 /*UpdateDebounce=*/std::chrono::steady_clock::duration::zero(),
 ASTRetentionPolicy());
@@ -184,7 +182,7 @@
 TEST_F(TUSchedulerTests, Debounce) {
   std::atomic CallbackCount(0);
   {
-TUScheduler S(getDefaultAsyncThreadsCount(),
+TUScheduler S(CDB, getDefaultAsyncThreadsCount(),
   /*StorePreamblesInMemory=*/true, captureDiags(),
   /*UpdateDebounce=*/std::chrono::seconds(1),
   ASTRetentionPolicy());
@@ -220,7 +218,7 @@
   {
 Notification InconsistentReadDone; // Must live longest.
 TUScheduler S(
-getDefaultAsyncThreadsCount(), /*StorePreamblesInMemory=*/true,
+CDB, getDefaultAsyncThreadsCount(), /*StorePreamblesInMemory=*/true,
 /*ASTCallbacks=*/nullptr,
 /*UpdateDebounce=*/std::chrono::steady_clock::duration::zero(),
 ASTRetentionPolicy());
@@ -277,7 +275,7 @@
   {
 Notification Proceed; // Ensure we schedule everything.
 TUScheduler S(
-getDefaultAsyncThreadsCount(), /*StorePreamblesInMemory=*/true,
+CDB, getDefaultAsyncThreadsCount(), /*StorePreamblesInMemory=*/true,
 /*ASTCallbacks=*/captureDiags(),
 /*UpdateDebounce=*/std::chrono::steady_clock::duration::zero(),
 ASTRetentionPolicy());
@@ -346,7 +344,7 @@
 
   // Run TUScheduler and collect some stats.
   {
-TUScheduler S(getDefaultAsyncThreadsCount(),
+TUScheduler S(CDB, getDefaultAsyncThreadsCount(),
   /*StorePreamblesInMemory=*/true, captureDiags(),
   /*UpdateDebounce=*/std::chrono::milliseconds(50),
   ASTRetentionPolicy());
@@ -437,10 +435,11 @@
   std::atomic BuiltASTCounter(0);
   ASTRetentionPolicy Policy;
   Policy.MaxRetainedASTs = 2;
-  TUScheduler S(
-  /*AsyncThreadsCount=*/1, /*StorePreambleInMemory=*/true,
-  /*ASTCallbacks=*/nullptr,
-  /*UpdateDebounce=*/std::chrono::steady_clock::duration::zero(), Policy);
+  TUScheduler S(CDB,
+/*AsyncThreadsCount=*/1, /*StorePreambleInMemory=*/true,
+/*ASTCallbacks=*/nullptr,
+/*UpdateDebounce=*/std::chrono::steady_clock::duration::zero(),
+Policy);
 
   llvm::StringLiteral SourceContents = R"cpp(
 int* a;
@@ -487,11 +486,11 @@
 }
 
 TEST_F(TUSchedulerTests, EmptyPreamble) {
-  TUScheduler S(
-  /*AsyncThreadsCount=*/4, /*StorePreambleInMemory=*/true,
-  /*ASTCallbacks=*/nullptr,
-  /*UpdateDebounce=*/std::chrono::steady_clock::duration::zero(),
-  ASTRetentionPolicy());
+  TUScheduler S(CDB,
+/*AsyncThreadsCount=*/4, /*StorePreambleInMemory=*/true,
+/*ASTCallbacks=*/nullptr,
+/*UpdateDebounce=*/std::chrono::steady_clock::duration::zero(),
+ASTRetentionPolicy());
 
   auto Foo = testPath("foo.cpp");
   auto Header = testPath("foo.h");
@@ -532,11 +531,11 @@
 TEST_F(TUSchedulerTests, RunWaitsForPreamble) {
   // Testing strategy: we update the file and schedule a few preamble reads at
   // the same time. All reads should get the same non-null preamble.
-  TUScheduler S(
-  /*AsyncThreadsCount=*/4, /*StorePreambleInMemory=*/true,
-  /*ASTCallbacks=*/nullptr,
-  

[PATCH] D59639: [clangd] Print template arguments helper

[Eric Liu via Phabricator via cfe-commits]

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

lgtm




Comment at: clang-tools-extra/clangd/AST.cpp:139
+  // location information.
+  printTemplateArgumentList(OS, Cls->getTemplateArgs().asArray(), Policy);
+}

Could you also add a test case for this with the current behavior and a FIXME?



Comment at: clang-tools-extra/unittests/clangd/PrintASTTests.cpp:47
+}
+std::vector TemplateArgs;
+const std::vector Points;

maybe `s/TemplateArgs/TemplateArgsAtPoints/` for clarity


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D59639



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

[PATCH] D60126: [clangd] Use identifiers in file as completion candidates when build is not ready.

[Eric Liu via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rCTE358159: [clangd] Use identifiers in file as completion 
candidates when build is not… (authored by ioeric, committed by ).

Changed prior to commit:
  https://reviews.llvm.org/D60126?vs=194652=194653#toc

Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D60126

Files:
  clangd/ClangdServer.cpp
  clangd/ClangdUnit.cpp
  clangd/CodeComplete.cpp
  clangd/CodeComplete.h
  clangd/Headers.cpp
  clangd/Headers.h
  clangd/SourceCode.cpp
  clangd/SourceCode.h
  clangd/index/SymbolOrigin.cpp
  clangd/index/SymbolOrigin.h
  unittests/clangd/ClangdTests.cpp
  unittests/clangd/CodeCompleteTests.cpp
  unittests/clangd/HeadersTests.cpp
  unittests/clangd/SourceCodeTests.cpp

Index: unittests/clangd/CodeCompleteTests.cpp
===
--- unittests/clangd/CodeCompleteTests.cpp
+++ unittests/clangd/CodeCompleteTests.cpp
@@ -20,6 +20,7 @@
 #include "TestTU.h"
 #include "index/MemIndex.h"
 #include "clang/Sema/CodeCompleteConsumer.h"
+#include "clang/Tooling/CompilationDatabase.h"
 #include "llvm/Support/Error.h"
 #include "llvm/Testing/Support/Error.h"
 #include "gmock/gmock.h"
@@ -138,6 +139,25 @@
  FilePath);
 }
 
+// Builds a server and runs code completion.
+// If IndexSymbols is non-empty, an index will be built and passed to opts.
+CodeCompleteResult completionsNoCompile(llvm::StringRef Text,
+std::vector IndexSymbols = {},
+clangd::CodeCompleteOptions Opts = {},
+PathRef FilePath = "foo.cpp") {
+  std::unique_ptr OverrideIndex;
+  if (!IndexSymbols.empty()) {
+assert(!Opts.Index && "both Index and IndexSymbols given!");
+OverrideIndex = memIndex(std::move(IndexSymbols));
+Opts.Index = OverrideIndex.get();
+  }
+
+  MockFSProvider FS;
+  Annotations Test(Text);
+  return codeComplete(FilePath, tooling::CompileCommand(), /*Preamble=*/nullptr,
+  Test.code(), Test.point(), FS.getFileSystem(), Opts);
+}
+
 Symbol withReferences(int N, Symbol S) {
   S.References = N;
   return S;
@@ -2401,6 +2421,33 @@
   UnorderedElementsAre(AllOf(Qualifier(""), Named("ABCDE";
 }
 
+TEST(NoCompileCompletionTest, Basic) {
+  auto Results = completionsNoCompile(R"cpp(
+void func() {
+  int xyz;
+  int abc;
+  ^
+}
+  )cpp");
+  EXPECT_THAT(Results.Completions,
+  UnorderedElementsAre(Named("void"), Named("func"), Named("int"),
+   Named("xyz"), Named("abc")));
+}
+
+TEST(NoCompileCompletionTest, WithFilter) {
+  auto Results = completionsNoCompile(R"cpp(
+void func() {
+  int sym1;
+  int sym2;
+  int xyz1;
+  int xyz2;
+  sy^
+}
+  )cpp");
+  EXPECT_THAT(Results.Completions,
+  UnorderedElementsAre(Named("sym1"), Named("sym2")));
+}
+
 } // namespace
 } // namespace clangd
 } // namespace clang
Index: unittests/clangd/ClangdTests.cpp
===
--- unittests/clangd/ClangdTests.cpp
+++ unittests/clangd/ClangdTests.cpp
@@ -535,12 +535,12 @@
   EXPECT_ERROR(runLocateSymbolAt(Server, FooCpp, Position()));
   EXPECT_ERROR(runFindDocumentHighlights(Server, FooCpp, Position()));
   EXPECT_ERROR(runRename(Server, FooCpp, Position(), "new_name"));
-  // FIXME: codeComplete and signatureHelp should also return errors when they
-  // can't parse the file.
+  // Identifier-based fallback completion.
   EXPECT_THAT(cantFail(runCodeComplete(Server, FooCpp, Position(),
clangd::CodeCompleteOptions()))
   .Completions,
-  IsEmpty());
+  ElementsAre(Field(::Name, "int"),
+  Field(::Name, "main")));
   auto SigHelp = runSignatureHelp(Server, FooCpp, Position());
   ASSERT_TRUE(bool(SigHelp)) << "signatureHelp returned an error";
   EXPECT_THAT(SigHelp->signatures, IsEmpty());
@@ -1066,10 +1066,11 @@
   ClangdServer Server(CDB, FS, DiagConsumer, ClangdServer::optsForTest());
 
   auto FooCpp = testPath("foo.cpp");
-  Annotations Code(R"cpp(
+   Annotations Code(R"cpp(
+namespace ns { int xyz; }
+using namespace ns;
 int main() {
-  int xyz;
-  xy^
+   xy^
 })cpp");
   FS.Files[FooCpp] = FooCpp;
 
@@ -1081,17 +1082,21 @@
   Server.addDocument(FooCpp, Code.code());
   ASSERT_TRUE(Server.blockUntilIdleForTest());
   auto Res = cantFail(runCodeComplete(Server, FooCpp, Code.point(), Opts));
-  EXPECT_THAT(Res.Completions, IsEmpty());
   EXPECT_EQ(Res.Context, CodeCompletionContext::CCC_Recovery);
+  // Identifier-based fallback completion doesn't know about "symbol" scope.
+  EXPECT_THAT(Res.Completions,
+  ElementsAre(AllOf(Field(::Name, "xyz"),
+  

[PATCH] D60126: [clangd] Use identifiers in file as completion candidates when build is not ready.

[Eric Liu via Phabricator via cfe-commits]

ioeric updated this revision to Diff 194652.
ioeric marked 2 inline comments as done.
ioeric added a comment.

- address comments


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D60126

Files:
  clangd/ClangdServer.cpp
  clangd/ClangdUnit.cpp
  clangd/CodeComplete.cpp
  clangd/CodeComplete.h
  clangd/Headers.cpp
  clangd/Headers.h
  clangd/SourceCode.cpp
  clangd/SourceCode.h
  clangd/index/SymbolOrigin.cpp
  clangd/index/SymbolOrigin.h
  unittests/clangd/ClangdTests.cpp
  unittests/clangd/CodeCompleteTests.cpp
  unittests/clangd/HeadersTests.cpp
  unittests/clangd/SourceCodeTests.cpp

Index: unittests/clangd/SourceCodeTests.cpp
===
--- unittests/clangd/SourceCodeTests.cpp
+++ unittests/clangd/SourceCodeTests.cpp
@@ -9,6 +9,7 @@
 #include "Context.h"
 #include "Protocol.h"
 #include "SourceCode.h"
+#include "clang/Format/Format.h"
 #include "llvm/Support/Error.h"
 #include "llvm/Support/raw_os_ostream.h"
 #include "llvm/Testing/Support/Error.h"
@@ -304,6 +305,23 @@
   }
 }
 
+TEST(SourceCodeTests, CollectIdentifiers) {
+  auto Style = format::getLLVMStyle();
+  auto IDs = collectIdentifiers(R"cpp(
+  #include "a.h"
+  void foo() { int xyz; int abc = xyz; return foo(); }
+  )cpp",
+Style);
+  EXPECT_EQ(IDs.size(), 7u);
+  EXPECT_EQ(IDs["include"], 1u);
+  EXPECT_EQ(IDs["void"], 1u);
+  EXPECT_EQ(IDs["int"], 2u);
+  EXPECT_EQ(IDs["xyz"], 2u);
+  EXPECT_EQ(IDs["abc"], 1u);
+  EXPECT_EQ(IDs["return"], 1u);
+  EXPECT_EQ(IDs["foo"], 2u);
+}
+
 } // namespace
 } // namespace clangd
 } // namespace clang
Index: unittests/clangd/HeadersTests.cpp
===
--- unittests/clangd/HeadersTests.cpp
+++ unittests/clangd/HeadersTests.cpp
@@ -90,7 +90,7 @@
 
 IncludeInserter Inserter(MainFile, /*Code=*/"", format::getLLVMStyle(),
  CDB.getCompileCommand(MainFile)->Directory,
- Clang->getPreprocessor().getHeaderSearchInfo());
+ >getPreprocessor().getHeaderSearchInfo());
 for (const auto  : Inclusions)
   Inserter.addExisting(Inc);
 auto Declaring = ToHeaderFile(Original);
@@ -110,7 +110,7 @@
 
 IncludeInserter Inserter(MainFile, /*Code=*/"", format::getLLVMStyle(),
  CDB.getCompileCommand(MainFile)->Directory,
- Clang->getPreprocessor().getHeaderSearchInfo());
+ >getPreprocessor().getHeaderSearchInfo());
 auto Edit = Inserter.insert(VerbatimHeader);
 Action.EndSourceFile();
 return Edit;
@@ -252,6 +252,24 @@
   EXPECT_TRUE(StringRef(Edit->newText).contains(""));
 }
 
+TEST(Headers, NoHeaderSearchInfo) {
+  std::string MainFile = testPath("main.cpp");
+  IncludeInserter Inserter(MainFile, /*Code=*/"", format::getLLVMStyle(),
+   /*BuildDir=*/"", /*HeaderSearchInfo=*/nullptr);
+
+  auto HeaderPath = testPath("sub/bar.h");
+  auto Declaring = HeaderFile{HeaderPath, /*Verbatim=*/false};
+  auto Inserting = HeaderFile{HeaderPath, /*Verbatim=*/false};
+  auto Verbatim = HeaderFile{"", /*Verbatim=*/true};
+
+  EXPECT_EQ(Inserter.calculateIncludePath(Declaring, Inserting),
+"\"" + HeaderPath + "\"");
+  EXPECT_EQ(Inserter.shouldInsertInclude(Declaring, Inserting), false);
+
+  EXPECT_EQ(Inserter.calculateIncludePath(Declaring, Verbatim), "");
+  EXPECT_EQ(Inserter.shouldInsertInclude(Declaring, Verbatim), true);
+}
+
 } // namespace
 } // namespace clangd
 } // namespace clang
Index: unittests/clangd/CodeCompleteTests.cpp
===
--- unittests/clangd/CodeCompleteTests.cpp
+++ unittests/clangd/CodeCompleteTests.cpp
@@ -20,6 +20,7 @@
 #include "TestTU.h"
 #include "index/MemIndex.h"
 #include "clang/Sema/CodeCompleteConsumer.h"
+#include "clang/Tooling/CompilationDatabase.h"
 #include "llvm/Support/Error.h"
 #include "llvm/Testing/Support/Error.h"
 #include "gmock/gmock.h"
@@ -138,6 +139,25 @@
  FilePath);
 }
 
+// Builds a server and runs code completion.
+// If IndexSymbols is non-empty, an index will be built and passed to opts.
+CodeCompleteResult completionsNoCompile(llvm::StringRef Text,
+std::vector IndexSymbols = {},
+clangd::CodeCompleteOptions Opts = {},
+PathRef FilePath = "foo.cpp") {
+  std::unique_ptr OverrideIndex;
+  if (!IndexSymbols.empty()) {
+assert(!Opts.Index && "both Index and IndexSymbols given!");
+OverrideIndex = memIndex(std::move(IndexSymbols));
+Opts.Index = OverrideIndex.get();
+  }
+
+  MockFSProvider FS;
+  Annotations Test(Text);
+  return codeComplete(FilePath, tooling::CompileCommand(), /*Preamble=*/nullptr,
+   

[PATCH] D60126: [clangd] Use identifiers in file as completion candidates when build is not ready.

[Eric Liu via Phabricator via cfe-commits]

ioeric added inline comments.



Comment at: clangd/CodeComplete.cpp:1360
 getQueryScopes(Recorder->CCContext, *Recorder->CCSema, Opts);
 if (!QueryScopes.empty())
   ScopeProximity.emplace(QueryScopes);

sammccall wrote:
> add this to the non-sema case too (though the if is always false for now), or 
> add a fixme?
> Worried about forgetting this.
Added a FIXME.



Comment at: clangd/Headers.cpp:196
+  if (!HeaderSearchInfo)
+return "\"" + InsertedHeader.File + "\"";
+  std::string Suggested = HeaderSearchInfo->suggestPathToFileForDiagnostics(

sammccall wrote:
> Do we expect this code path to ever be called?
> We may want to assert or elog here, depending on how this can be hit.
This can be hit when we start serving index symbols in fallback mode 
completion. `shouldInsertInclude` will return false, but the calculation will 
be attempted regardless. We can probably elog this, but the number of log 
messages would be O(# of index symbols), which I worry might be too spammy.


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D60126



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

[PATCH] D60126: [clangd] Use identifiers in file as completion candidates when build is not ready.

[Eric Liu via Phabricator via cfe-commits]

ioeric updated this revision to Diff 194647.
ioeric marked 14 inline comments as done.
ioeric added a comment.

- address review comments


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D60126

Files:
  clangd/ClangdServer.cpp
  clangd/ClangdUnit.cpp
  clangd/CodeComplete.cpp
  clangd/CodeComplete.h
  clangd/Headers.cpp
  clangd/Headers.h
  clangd/SourceCode.cpp
  clangd/SourceCode.h
  clangd/index/SymbolOrigin.cpp
  clangd/index/SymbolOrigin.h
  unittests/clangd/ClangdTests.cpp
  unittests/clangd/CodeCompleteTests.cpp
  unittests/clangd/HeadersTests.cpp
  unittests/clangd/SourceCodeTests.cpp

Index: unittests/clangd/SourceCodeTests.cpp
===
--- unittests/clangd/SourceCodeTests.cpp
+++ unittests/clangd/SourceCodeTests.cpp
@@ -9,6 +9,7 @@
 #include "Context.h"
 #include "Protocol.h"
 #include "SourceCode.h"
+#include "clang/Format/Format.h"
 #include "llvm/Support/Error.h"
 #include "llvm/Support/raw_os_ostream.h"
 #include "llvm/Testing/Support/Error.h"
@@ -304,6 +305,23 @@
   }
 }
 
+TEST(SourceCodeTests, CollectIdentifiers) {
+  auto Style = format::getLLVMStyle();
+  auto IDs = collectIdentifiers(R"cpp(
+  #include "a.h"
+  void foo() { int xyz; int abc = xyz; return foo(); }
+  )cpp",
+Style);
+  EXPECT_EQ(IDs.size(), 7u);
+  EXPECT_EQ(IDs["include"], 1u);
+  EXPECT_EQ(IDs["void"], 1u);
+  EXPECT_EQ(IDs["int"], 2u);
+  EXPECT_EQ(IDs["xyz"], 2u);
+  EXPECT_EQ(IDs["abc"], 1u);
+  EXPECT_EQ(IDs["return"], 1u);
+  EXPECT_EQ(IDs["foo"], 2u);
+}
+
 } // namespace
 } // namespace clangd
 } // namespace clang
Index: unittests/clangd/HeadersTests.cpp
===
--- unittests/clangd/HeadersTests.cpp
+++ unittests/clangd/HeadersTests.cpp
@@ -90,7 +90,7 @@
 
 IncludeInserter Inserter(MainFile, /*Code=*/"", format::getLLVMStyle(),
  CDB.getCompileCommand(MainFile)->Directory,
- Clang->getPreprocessor().getHeaderSearchInfo());
+ >getPreprocessor().getHeaderSearchInfo());
 for (const auto  : Inclusions)
   Inserter.addExisting(Inc);
 auto Declaring = ToHeaderFile(Original);
@@ -110,7 +110,7 @@
 
 IncludeInserter Inserter(MainFile, /*Code=*/"", format::getLLVMStyle(),
  CDB.getCompileCommand(MainFile)->Directory,
- Clang->getPreprocessor().getHeaderSearchInfo());
+ >getPreprocessor().getHeaderSearchInfo());
 auto Edit = Inserter.insert(VerbatimHeader);
 Action.EndSourceFile();
 return Edit;
@@ -252,6 +252,24 @@
   EXPECT_TRUE(StringRef(Edit->newText).contains(""));
 }
 
+TEST(Headers, NoHeaderSearchInfo) {
+  std::string MainFile = testPath("main.cpp");
+  IncludeInserter Inserter(MainFile, /*Code=*/"", format::getLLVMStyle(),
+   /*BuildDir=*/"", /*HeaderSearchInfo=*/nullptr);
+
+  auto HeaderPath = testPath("sub/bar.h");
+  auto Declaring = HeaderFile{HeaderPath, /*Verbatim=*/false};
+  auto Inserting = HeaderFile{HeaderPath, /*Verbatim=*/false};
+  auto Verbatim = HeaderFile{"", /*Verbatim=*/true};
+
+  EXPECT_EQ(Inserter.calculateIncludePath(Declaring, Inserting),
+"\"" + HeaderPath + "\"");
+  EXPECT_EQ(Inserter.shouldInsertInclude(Declaring, Inserting), false);
+
+  EXPECT_EQ(Inserter.calculateIncludePath(Declaring, Verbatim), "");
+  EXPECT_EQ(Inserter.shouldInsertInclude(Declaring, Verbatim), true);
+}
+
 } // namespace
 } // namespace clangd
 } // namespace clang
Index: unittests/clangd/CodeCompleteTests.cpp
===
--- unittests/clangd/CodeCompleteTests.cpp
+++ unittests/clangd/CodeCompleteTests.cpp
@@ -20,6 +20,7 @@
 #include "TestTU.h"
 #include "index/MemIndex.h"
 #include "clang/Sema/CodeCompleteConsumer.h"
+#include "clang/Tooling/CompilationDatabase.h"
 #include "llvm/Support/Error.h"
 #include "llvm/Testing/Support/Error.h"
 #include "gmock/gmock.h"
@@ -138,6 +139,25 @@
  FilePath);
 }
 
+// Builds a server and runs code completion.
+// If IndexSymbols is non-empty, an index will be built and passed to opts.
+CodeCompleteResult completionsNoCompile(llvm::StringRef Text,
+std::vector IndexSymbols = {},
+clangd::CodeCompleteOptions Opts = {},
+PathRef FilePath = "foo.cpp") {
+  std::unique_ptr OverrideIndex;
+  if (!IndexSymbols.empty()) {
+assert(!Opts.Index && "both Index and IndexSymbols given!");
+OverrideIndex = memIndex(std::move(IndexSymbols));
+Opts.Index = OverrideIndex.get();
+  }
+
+  MockFSProvider FS;
+  Annotations Test(Text);
+  return codeComplete(FilePath, tooling::CompileCommand(), /*Preamble=*/nullptr,
+   

[PATCH] D60503: [clangd] Don't insert extra namespace qualifiers when Sema gets lost.

[Eric Liu via Phabricator via cfe-commits]

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

Feel free to land this. I'll rebase D60126  on 
this when it's in.




Comment at: clangd/CodeComplete.cpp:580
+  // Case 3: sema saw and resolved a scope qualifier.
+  if (SemaSpecifier && SemaSpecifier->isValid())
+return {Scopes.scopesForIndexQuery(), false};

nit: Sema Specifier can't be nullptr at this point.


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D60503



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

[PATCH] D60500: [clangd] Refactor speculateCompletionFilter and also extract scope.

[Eric Liu via Phabricator via cfe-commits]

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

lgtm




Comment at: clangd/CodeComplete.cpp:1542
+  assert(Offset <= Content.size());
+  StringRef Suffix = Content.take_front(Offset);
+  CompletionPrefix Result;

`Suffix` is actually a prefix of Content? This seems a bit confusing...


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D60500



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

[PATCH] D60126: [clangd] Use identifiers in file as completion candidates when build is not ready.

[Eric Liu via Phabricator via cfe-commits]

ioeric added a comment.

Thanks for the review!




Comment at: clangd/CodeComplete.cpp:1320
+llvm::IntrusiveRefCntPtr VFS) && {
+auto CompletionFilter = speculateCompletionFilter(Content, Pos);
+if (!CompletionFilter) {

sammccall wrote:
> hmm, this function should likely also move to SourceCode, use Offset instead 
> of Position, and move to SourceCode.h. But probably a different patch.
ack



Comment at: clangd/CodeComplete.cpp:1341
+
+// Carve out the typed filter from the content so that we don't treat it as
+// an identifier.

sammccall wrote:
> you could just erase the typed filter from the suggestion list.
> (It may be a valid word spelled elsewhere, but there's no point suggesting it)
This is following conventions of other sources. Both index and sema provide 
results for fully-typed names. Considering that users might be already used to 
this, and completion results tend to give reassurance for correctly typed 
names, I am inclined to keep the typed filter if it's seen somewhere else in 
the file.


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D60126



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

[PATCH] D60126: [clangd] Use identifiers in file as completion candidates when build is not ready.

[Eric Liu via Phabricator via cfe-commits]

ioeric updated this revision to Diff 194315.
ioeric added a comment.

- minor cleanup


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D60126

Files:
  clangd/ClangdServer.cpp
  clangd/ClangdUnit.cpp
  clangd/CodeComplete.cpp
  clangd/CodeComplete.h
  clangd/Headers.cpp
  clangd/Headers.h
  clangd/SourceCode.cpp
  clangd/SourceCode.h
  unittests/clangd/ClangdTests.cpp
  unittests/clangd/CodeCompleteTests.cpp
  unittests/clangd/HeadersTests.cpp
  unittests/clangd/SourceCodeTests.cpp

Index: unittests/clangd/SourceCodeTests.cpp
===
--- unittests/clangd/SourceCodeTests.cpp
+++ unittests/clangd/SourceCodeTests.cpp
@@ -9,6 +9,7 @@
 #include "Context.h"
 #include "Protocol.h"
 #include "SourceCode.h"
+#include "clang/Format/Format.h"
 #include "llvm/Support/Error.h"
 #include "llvm/Support/raw_os_ostream.h"
 #include "llvm/Testing/Support/Error.h"
@@ -304,6 +305,23 @@
   }
 }
 
+TEST(SourceCodeTests, CollectIdentifiers) {
+  auto Style = format::getLLVMStyle();
+  auto IDs = collectIdentifiers(R"cpp(
+  #include "a.h"
+  void foo() { int xyz; int abc = xyz; return foo(); }
+  )cpp",
+Style);
+  EXPECT_EQ(IDs.size(), 7u);
+  EXPECT_EQ(IDs["include"], 1u);
+  EXPECT_EQ(IDs["void"], 1u);
+  EXPECT_EQ(IDs["int"], 2u);
+  EXPECT_EQ(IDs["xyz"], 2u);
+  EXPECT_EQ(IDs["abc"], 1u);
+  EXPECT_EQ(IDs["return"], 1u);
+  EXPECT_EQ(IDs["foo"], 2u);
+}
+
 } // namespace
 } // namespace clangd
 } // namespace clang
Index: unittests/clangd/HeadersTests.cpp
===
--- unittests/clangd/HeadersTests.cpp
+++ unittests/clangd/HeadersTests.cpp
@@ -90,7 +90,7 @@
 
 IncludeInserter Inserter(MainFile, /*Code=*/"", format::getLLVMStyle(),
  CDB.getCompileCommand(MainFile)->Directory,
- Clang->getPreprocessor().getHeaderSearchInfo());
+ >getPreprocessor().getHeaderSearchInfo());
 for (const auto  : Inclusions)
   Inserter.addExisting(Inc);
 auto Declaring = ToHeaderFile(Original);
@@ -110,7 +110,7 @@
 
 IncludeInserter Inserter(MainFile, /*Code=*/"", format::getLLVMStyle(),
  CDB.getCompileCommand(MainFile)->Directory,
- Clang->getPreprocessor().getHeaderSearchInfo());
+ >getPreprocessor().getHeaderSearchInfo());
 auto Edit = Inserter.insert(VerbatimHeader);
 Action.EndSourceFile();
 return Edit;
@@ -252,6 +252,24 @@
   EXPECT_TRUE(StringRef(Edit->newText).contains(""));
 }
 
+TEST(Headers, NoHeaderSearchInfo) {
+  std::string MainFile = testPath("main.cpp");
+  IncludeInserter Inserter(MainFile, /*Code=*/"", format::getLLVMStyle(),
+   /*BuildDir=*/"", /*HeaderSearchInfo=*/nullptr);
+
+  auto HeaderPath = testPath("sub/bar.h");
+  auto Declaring = HeaderFile{HeaderPath, /*Verbatim=*/false};
+  auto Inserting = HeaderFile{HeaderPath, /*Verbatim=*/false};
+  auto Verbatim = HeaderFile{"", /*Verbatim=*/true};
+
+  EXPECT_EQ(Inserter.calculateIncludePath(Declaring, Inserting),
+"\"" + HeaderPath + "\"");
+  EXPECT_EQ(Inserter.shouldInsertInclude(Declaring, Inserting), false);
+
+  EXPECT_EQ(Inserter.calculateIncludePath(Declaring, Verbatim), "");
+  EXPECT_EQ(Inserter.shouldInsertInclude(Declaring, Verbatim), true);
+}
+
 } // namespace
 } // namespace clangd
 } // namespace clang
Index: unittests/clangd/CodeCompleteTests.cpp
===
--- unittests/clangd/CodeCompleteTests.cpp
+++ unittests/clangd/CodeCompleteTests.cpp
@@ -20,6 +20,7 @@
 #include "TestTU.h"
 #include "index/MemIndex.h"
 #include "clang/Sema/CodeCompleteConsumer.h"
+#include "clang/Tooling/CompilationDatabase.h"
 #include "llvm/Support/Error.h"
 #include "llvm/Testing/Support/Error.h"
 #include "gmock/gmock.h"
@@ -32,7 +33,6 @@
 using ::llvm::Failed;
 using ::testing::AllOf;
 using ::testing::Contains;
-using ::testing::Each;
 using ::testing::ElementsAre;
 using ::testing::Field;
 using ::testing::HasSubstr;
@@ -139,6 +139,25 @@
  FilePath);
 }
 
+// Builds a server and runs code completion.
+// If IndexSymbols is non-empty, an index will be built and passed to opts.
+CodeCompleteResult completionsNoCompile(llvm::StringRef Text,
+std::vector IndexSymbols = {},
+clangd::CodeCompleteOptions Opts = {},
+PathRef FilePath = "foo.cpp") {
+  std::unique_ptr OverrideIndex;
+  if (!IndexSymbols.empty()) {
+assert(!Opts.Index && "both Index and IndexSymbols given!");
+OverrideIndex = memIndex(std::move(IndexSymbols));
+Opts.Index = OverrideIndex.get();
+  }
+
+  MockFSProvider FS;
+  Annotations Test(Text);
+  

[PATCH] D60126: [clangd] Use identifiers in file as completion candidates when build is not ready.

[Eric Liu via Phabricator via cfe-commits]

ioeric updated this revision to Diff 194311.
ioeric marked 9 inline comments as done.
ioeric added a comment.

- address review comments


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D60126

Files:
  clangd/ClangdServer.cpp
  clangd/ClangdUnit.cpp
  clangd/CodeComplete.cpp
  clangd/CodeComplete.h
  clangd/Headers.cpp
  clangd/Headers.h
  clangd/SourceCode.cpp
  clangd/SourceCode.h
  unittests/clangd/ClangdTests.cpp
  unittests/clangd/CodeCompleteTests.cpp
  unittests/clangd/HeadersTests.cpp
  unittests/clangd/SourceCodeTests.cpp

Index: unittests/clangd/SourceCodeTests.cpp
===
--- unittests/clangd/SourceCodeTests.cpp
+++ unittests/clangd/SourceCodeTests.cpp
@@ -9,6 +9,7 @@
 #include "Context.h"
 #include "Protocol.h"
 #include "SourceCode.h"
+#include "clang/Format/Format.h"
 #include "llvm/Support/Error.h"
 #include "llvm/Support/raw_os_ostream.h"
 #include "llvm/Testing/Support/Error.h"
@@ -304,6 +305,23 @@
   }
 }
 
+TEST(SourceCodeTests, CollectIdentifiers) {
+  auto Style = format::getLLVMStyle();
+  auto IDs = collectIdentifiers(R"cpp(
+  #include "a.h"
+  void foo() { int xyz; int abc = xyz; return foo(); }
+  )cpp",
+Style);
+  EXPECT_EQ(IDs.size(), 7u);
+  EXPECT_EQ(IDs["include"], 1u);
+  EXPECT_EQ(IDs["void"], 1u);
+  EXPECT_EQ(IDs["int"], 2u);
+  EXPECT_EQ(IDs["xyz"], 2u);
+  EXPECT_EQ(IDs["abc"], 1u);
+  EXPECT_EQ(IDs["return"], 1u);
+  EXPECT_EQ(IDs["foo"], 2u);
+}
+
 } // namespace
 } // namespace clangd
 } // namespace clang
Index: unittests/clangd/HeadersTests.cpp
===
--- unittests/clangd/HeadersTests.cpp
+++ unittests/clangd/HeadersTests.cpp
@@ -90,7 +90,7 @@
 
 IncludeInserter Inserter(MainFile, /*Code=*/"", format::getLLVMStyle(),
  CDB.getCompileCommand(MainFile)->Directory,
- Clang->getPreprocessor().getHeaderSearchInfo());
+ >getPreprocessor().getHeaderSearchInfo());
 for (const auto  : Inclusions)
   Inserter.addExisting(Inc);
 auto Declaring = ToHeaderFile(Original);
@@ -110,7 +110,7 @@
 
 IncludeInserter Inserter(MainFile, /*Code=*/"", format::getLLVMStyle(),
  CDB.getCompileCommand(MainFile)->Directory,
- Clang->getPreprocessor().getHeaderSearchInfo());
+ >getPreprocessor().getHeaderSearchInfo());
 auto Edit = Inserter.insert(VerbatimHeader);
 Action.EndSourceFile();
 return Edit;
@@ -252,6 +252,24 @@
   EXPECT_TRUE(StringRef(Edit->newText).contains(""));
 }
 
+TEST(Headers, NoHeaderSearchInfo) {
+  std::string MainFile = testPath("main.cpp");
+  IncludeInserter Inserter(MainFile, /*Code=*/"", format::getLLVMStyle(),
+   /*BuildDir=*/"", /*HeaderSearchInfo=*/nullptr);
+
+  auto HeaderPath = testPath("sub/bar.h");
+  auto Declaring = HeaderFile{HeaderPath, /*Verbatim=*/false};
+  auto Inserting = HeaderFile{HeaderPath, /*Verbatim=*/false};
+  auto Verbatim = HeaderFile{"", /*Verbatim=*/true};
+
+  EXPECT_EQ(Inserter.calculateIncludePath(Declaring, Inserting),
+"\"" + HeaderPath + "\"");
+  EXPECT_EQ(Inserter.shouldInsertInclude(Declaring, Inserting), false);
+
+  EXPECT_EQ(Inserter.calculateIncludePath(Declaring, Verbatim), "");
+  EXPECT_EQ(Inserter.shouldInsertInclude(Declaring, Verbatim), true);
+}
+
 } // namespace
 } // namespace clangd
 } // namespace clang
Index: unittests/clangd/CodeCompleteTests.cpp
===
--- unittests/clangd/CodeCompleteTests.cpp
+++ unittests/clangd/CodeCompleteTests.cpp
@@ -20,6 +20,7 @@
 #include "TestTU.h"
 #include "index/MemIndex.h"
 #include "clang/Sema/CodeCompleteConsumer.h"
+#include "clang/Tooling/CompilationDatabase.h"
 #include "llvm/Support/Error.h"
 #include "llvm/Testing/Support/Error.h"
 #include "gmock/gmock.h"
@@ -32,7 +33,6 @@
 using ::llvm::Failed;
 using ::testing::AllOf;
 using ::testing::Contains;
-using ::testing::Each;
 using ::testing::ElementsAre;
 using ::testing::Field;
 using ::testing::HasSubstr;
@@ -139,6 +139,25 @@
  FilePath);
 }
 
+// Builds a server and runs code completion.
+// If IndexSymbols is non-empty, an index will be built and passed to opts.
+CodeCompleteResult completionsNoCompile(llvm::StringRef Text,
+std::vector IndexSymbols = {},
+clangd::CodeCompleteOptions Opts = {},
+PathRef FilePath = "foo.cpp") {
+  std::unique_ptr OverrideIndex;
+  if (!IndexSymbols.empty()) {
+assert(!Opts.Index && "both Index and IndexSymbols given!");
+OverrideIndex = memIndex(std::move(IndexSymbols));
+Opts.Index = OverrideIndex.get();
+  }
+
+  

[PATCH] D60409: [clangd] Add -header-insertion=never flag to disable include insertion in code completion

[Eric Liu via Phabricator via cfe-commits]

ioeric added a comment.

lg!




Comment at: clangd/CodeComplete.cpp:1176
   // This is available after Sema has run.
-  llvm::Optional Inserter;  // Available during runWithSema.
+  llvm::Optional Inserter;  // Optional during runWithSema.
   llvm::Optional FileProximity; // Initialized once Sema runs.

Why optional? In the current implementation, it's always initialized.


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D60409



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

[PATCH] D59811: [clangd] Add fallback mode for code completion when compile command or preamble is not ready.

[Eric Liu via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL357916: [clangd] Add fallback mode for code completion when 
compile command or preamble… (authored by ioeric, committed by ).
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.

Repository:
  rL LLVM

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

https://reviews.llvm.org/D59811

Files:
  clang-tools-extra/trunk/clangd/ClangdServer.cpp
  clang-tools-extra/trunk/clangd/CodeComplete.h
  clang-tools-extra/trunk/clangd/TUScheduler.cpp
  clang-tools-extra/trunk/clangd/TUScheduler.h
  clang-tools-extra/trunk/clangd/tool/ClangdMain.cpp
  clang-tools-extra/trunk/unittests/clangd/ClangdTests.cpp

Index: clang-tools-extra/trunk/unittests/clangd/ClangdTests.cpp
===
--- clang-tools-extra/trunk/unittests/clangd/ClangdTests.cpp
+++ clang-tools-extra/trunk/unittests/clangd/ClangdTests.cpp
@@ -13,8 +13,10 @@
 #include "Matchers.h"
 #include "SyncAPI.h"
 #include "TestFS.h"
+#include "Threading.h"
 #include "URI.h"
 #include "clang/Config/config.h"
+#include "clang/Sema/CodeCompleteConsumer.h"
 #include "llvm/ADT/SmallVector.h"
 #include "llvm/ADT/StringMap.h"
 #include "llvm/Support/Errc.h"
@@ -36,7 +38,6 @@
 namespace {
 
 using ::testing::ElementsAre;
-using ::testing::Eq;
 using ::testing::Field;
 using ::testing::Gt;
 using ::testing::IsEmpty;
@@ -1058,6 +1059,41 @@
   EXPECT_NE(Result, "");
 }
 
+TEST_F(ClangdVFSTest, FallbackWhenPreambleIsNotReady) {
+  MockFSProvider FS;
+  ErrorCheckingDiagConsumer DiagConsumer;
+  MockCompilationDatabase CDB;
+  ClangdServer Server(CDB, FS, DiagConsumer, ClangdServer::optsForTest());
+
+  auto FooCpp = testPath("foo.cpp");
+  Annotations Code(R"cpp(
+int main() {
+  int xyz;
+  xy^
+})cpp");
+  FS.Files[FooCpp] = FooCpp;
+
+  auto Opts = clangd::CodeCompleteOptions();
+  Opts.AllowFallback = true;
+
+  // This will make compile command broken and preamble absent.
+  CDB.ExtraClangFlags = {"yolo.cc"};
+  Server.addDocument(FooCpp, Code.code());
+  ASSERT_TRUE(Server.blockUntilIdleForTest());
+  auto Res = cantFail(runCodeComplete(Server, FooCpp, Code.point(), Opts));
+  EXPECT_THAT(Res.Completions, IsEmpty());
+  EXPECT_EQ(Res.Context, CodeCompletionContext::CCC_Recovery);
+
+  // Make the compile command work again.
+  CDB.ExtraClangFlags = {"-std=c++11"};
+  Server.addDocument(FooCpp, Code.code());
+  ASSERT_TRUE(Server.blockUntilIdleForTest());
+  EXPECT_THAT(cantFail(runCodeComplete(Server, FooCpp, Code.point(),
+   Opts))
+  .Completions,
+  ElementsAre(Field(::Name, "xyz")));
+}
+
 } // namespace
 } // namespace clangd
 } // namespace clang
Index: clang-tools-extra/trunk/clangd/ClangdServer.cpp
===
--- clang-tools-extra/trunk/clangd/ClangdServer.cpp
+++ clang-tools-extra/trunk/clangd/ClangdServer.cpp
@@ -11,7 +11,9 @@
 #include "CodeComplete.h"
 #include "FindSymbols.h"
 #include "Headers.h"
+#include "Protocol.h"
 #include "SourceCode.h"
+#include "TUScheduler.h"
 #include "Trace.h"
 #include "index/CanonicalIncludes.h"
 #include "index/FileIndex.h"
@@ -21,6 +23,7 @@
 #include "clang/Frontend/CompilerInstance.h"
 #include "clang/Frontend/CompilerInvocation.h"
 #include "clang/Lex/Preprocessor.h"
+#include "clang/Sema/CodeCompleteConsumer.h"
 #include "clang/Tooling/CompilationDatabase.h"
 #include "clang/Tooling/Core/Replacement.h"
 #include "clang/Tooling/Refactoring/RefactoringResultConsumer.h"
@@ -152,6 +155,7 @@
   if (ClangTidyOptProvider)
 Opts.ClangTidyOpts = ClangTidyOptProvider->getOptions(File);
   Opts.SuggestMissingIncludes = SuggestMissingIncludes;
+
   // FIXME: some build systems like Bazel will take time to preparing
   // environment to build the file, it would be nice if we could emit a
   // "PreparingBuild" status to inform users, it is non-trivial given the
@@ -183,6 +187,18 @@
   return CB(IP.takeError());
 if (isCancelled())
   return CB(llvm::make_error());
+if (!IP->Preamble) {
+  vlog("File {0} is not ready for code completion. Enter fallback mode.",
+   File);
+  CodeCompleteResult CCR;
+  CCR.Context = CodeCompletionContext::CCC_Recovery;
+
+  // FIXME: perform simple completion e.g. using identifiers in the current
+  // file and symbols in the index.
+  // FIXME: let clients know that we've entered fallback mode.
+
+  return CB(std::move(CCR));
+}
 
 llvm::Optional SpecFuzzyFind;
 if (CodeCompleteOpts.Index && CodeCompleteOpts.SpeculativeIndexRequest) {
@@ -214,7 +230,9 @@
   };
 
   // We use a potentially-stale preamble because latency is critical here.
-  WorkScheduler.runWithPreamble("CodeComplete", File, TUScheduler::Stale,
+  WorkScheduler.runWithPreamble("CodeComplete", File,
+Opts.AllowFallback ? 

[PATCH] D59811: [clangd] Add fallback mode for code completion when compile command or preamble is not ready.

[Eric Liu via Phabricator via cfe-commits]

ioeric updated this revision to Diff 194141.
ioeric marked 11 inline comments as done.
ioeric added a comment.

- address review comments


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D59811

Files:
  clangd/ClangdServer.cpp
  clangd/CodeComplete.h
  clangd/TUScheduler.cpp
  clangd/TUScheduler.h
  clangd/tool/ClangdMain.cpp
  unittests/clangd/ClangdTests.cpp

Index: unittests/clangd/ClangdTests.cpp
===
--- unittests/clangd/ClangdTests.cpp
+++ unittests/clangd/ClangdTests.cpp
@@ -13,8 +13,10 @@
 #include "Matchers.h"
 #include "SyncAPI.h"
 #include "TestFS.h"
+#include "Threading.h"
 #include "URI.h"
 #include "clang/Config/config.h"
+#include "clang/Sema/CodeCompleteConsumer.h"
 #include "llvm/ADT/SmallVector.h"
 #include "llvm/ADT/StringMap.h"
 #include "llvm/Support/Errc.h"
@@ -36,7 +38,6 @@
 namespace {
 
 using ::testing::ElementsAre;
-using ::testing::Eq;
 using ::testing::Field;
 using ::testing::Gt;
 using ::testing::IsEmpty;
@@ -1058,6 +1059,41 @@
   EXPECT_NE(Result, "");
 }
 
+TEST_F(ClangdVFSTest, FallbackWhenPreambleIsNotReady) {
+  MockFSProvider FS;
+  ErrorCheckingDiagConsumer DiagConsumer;
+  MockCompilationDatabase CDB;
+  ClangdServer Server(CDB, FS, DiagConsumer, ClangdServer::optsForTest());
+
+  auto FooCpp = testPath("foo.cpp");
+  Annotations Code(R"cpp(
+int main() {
+  int xyz;
+  xy^
+})cpp");
+  FS.Files[FooCpp] = FooCpp;
+
+  auto Opts = clangd::CodeCompleteOptions();
+  Opts.AllowFallback = true;
+
+  // This will make compile command broken and preamble absent.
+  CDB.ExtraClangFlags = {"yolo.cc"};
+  Server.addDocument(FooCpp, Code.code());
+  ASSERT_TRUE(Server.blockUntilIdleForTest());
+  auto Res = cantFail(runCodeComplete(Server, FooCpp, Code.point(), Opts));
+  EXPECT_THAT(Res.Completions, IsEmpty());
+  EXPECT_EQ(Res.Context, CodeCompletionContext::CCC_Recovery);
+
+  // Make the compile command work again.
+  CDB.ExtraClangFlags = {"-std=c++11"};
+  Server.addDocument(FooCpp, Code.code());
+  ASSERT_TRUE(Server.blockUntilIdleForTest());
+  EXPECT_THAT(cantFail(runCodeComplete(Server, FooCpp, Code.point(),
+   Opts))
+  .Completions,
+  ElementsAre(Field(::Name, "xyz")));
+}
+
 } // namespace
 } // namespace clangd
 } // namespace clang
Index: clangd/tool/ClangdMain.cpp
===
--- clangd/tool/ClangdMain.cpp
+++ clangd/tool/ClangdMain.cpp
@@ -231,6 +231,14 @@
 "Offsets are in UTF-16 code units")),
 llvm::cl::init(OffsetEncoding::UnsupportedEncoding));
 
+static llvm::cl::opt AllowFallbackCompletion(
+"allow-fallback-completion",
+llvm::cl::desc(
+"Allow falling back to code completion without compiling files (using "
+"identifiers and symbol indexes), when file cannot be built or the "
+"build is not ready."),
+llvm::cl::init(false));
+
 namespace {
 
 /// \brief Supports a test URI scheme with relaxed constraints for lit tests.
@@ -437,6 +445,7 @@
   CCOpts.SpeculativeIndexRequest = Opts.StaticIndex;
   CCOpts.EnableFunctionArgSnippets = EnableFunctionArgSnippets;
   CCOpts.AllScopes = AllScopesCompletion;
+  CCOpts.AllowFallback = AllowFallbackCompletion;
 
   RealFileSystemProvider FSProvider;
   // Initialize and run ClangdLSPServer.
Index: clangd/TUScheduler.h
===
--- clangd/TUScheduler.h
+++ clangd/TUScheduler.h
@@ -13,7 +13,9 @@
 #include "Function.h"
 #include "Threading.h"
 #include "index/CanonicalIncludes.h"
+#include "llvm/ADT/Optional.h"
 #include "llvm/ADT/StringMap.h"
+#include "llvm/ADT/StringRef.h"
 #include 
 
 namespace clang {
@@ -32,6 +34,7 @@
 struct InputsAndPreamble {
   llvm::StringRef Contents;
   const tooling::CompileCommand 
+  // This can be nullptr if no preamble is availble.
   const PreambleData *Preamble;
 };
 
@@ -178,10 +181,14 @@
 ///   reading source code from headers.
 /// This is the fastest option, usually a preamble is available immediately.
 Stale,
+/// Besides accepting stale preamble, this also allow preamble to be absent
+/// (not ready or failed to build).
+StaleOrAbsent,
   };
+
   /// Schedule an async read of the preamble.
-  /// If there's no preamble yet (because the file was just opened), we'll wait
-  /// for it to build. The result may be null if it fails to build or is empty.
+  /// If there's no up-to-date preamble, we follow the PreambleConsistency
+  /// policy.
   /// If an error occurs, it is forwarded to the \p Action callback.
   /// Context cancellation is ignored and should be handled by the Action.
   /// (In practice, the Action is almost always executed immediately).
Index: clangd/TUScheduler.cpp

[PATCH] D59811: [clangd] Add fallback mode for code completion when compile command or preamble is not ready.

[Eric Liu via Phabricator via cfe-commits]

ioeric added inline comments.



Comment at: clangd/TUScheduler.cpp:881
+  // asynchronous mode, as TU update should finish before this is run.
+  if (!It->second->Worker->isFirstPreambleBuilt() &&
+  Consistency == StaleOrAbsent && PreambleTasks) {

sammccall wrote:
> Does this need to be a special case at the top of 
> TUScheduler::runWithPreamble, rather than unified with the rest of the body?
> 
> e.g. the `if (!PreambleTasks)` appears to block appears to handle that case 
> correctly already, and the `Consistency == Consistent` block won't trigger.
> I think all you need to do is make the `Worker->waitForFirstPreamble()` line 
> conditional?
> 
> (This means no changes to ASTWorker, Notification etc)
whoops! absolutely right!



Comment at: unittests/clangd/ClangdTests.cpp:1080
+  // This will make compile command broken and preamble absent.
+  CDB.ExtraClangFlags = {"yolo.cc"};
+  Server.addDocument(FooCpp, Code.code());

sammccall wrote:
> (`-this-flag-does-not-exist` might be clearer)
This would still cause preamble to be built:
```
Updating file /clangd-test/foo.cpp with command [/clangd-test] clang 
-this-flag-does-not-exist /clangd-test/foo.cpp
Ignored diagnostic. unknown argument: '-this-flag-does-not-exist'
```


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D59811



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

[PATCH] D59811: [clangd] Add fallback mode for code completion when compile command or preamble is not ready.

[Eric Liu via Phabricator via cfe-commits]

ioeric updated this revision to Diff 194132.
ioeric added a comment.

- Fix unit test


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D59811

Files:
  clangd/ClangdServer.cpp
  clangd/CodeComplete.h
  clangd/TUScheduler.cpp
  clangd/TUScheduler.h
  clangd/Threading.h
  clangd/tool/ClangdMain.cpp
  unittests/clangd/ClangdTests.cpp

Index: unittests/clangd/ClangdTests.cpp
===
--- unittests/clangd/ClangdTests.cpp
+++ unittests/clangd/ClangdTests.cpp
@@ -13,8 +13,10 @@
 #include "Matchers.h"
 #include "SyncAPI.h"
 #include "TestFS.h"
+#include "Threading.h"
 #include "URI.h"
 #include "clang/Config/config.h"
+#include "clang/Sema/CodeCompleteConsumer.h"
 #include "llvm/ADT/SmallVector.h"
 #include "llvm/ADT/StringMap.h"
 #include "llvm/Support/Errc.h"
@@ -36,7 +38,6 @@
 namespace {
 
 using ::testing::ElementsAre;
-using ::testing::Eq;
 using ::testing::Field;
 using ::testing::Gt;
 using ::testing::IsEmpty;
@@ -1058,6 +1059,41 @@
   EXPECT_NE(Result, "");
 }
 
+TEST_F(ClangdVFSTest, FallbackWhenPreambleIsNotReady) {
+  MockFSProvider FS;
+  ErrorCheckingDiagConsumer DiagConsumer;
+  MockCompilationDatabase CDB;
+  ClangdServer Server(CDB, FS, DiagConsumer, ClangdServer::optsForTest());
+
+  auto FooCpp = testPath("foo.cpp");
+  Annotations Code(R"cpp(
+int main() {
+  int xyz;
+  xy^
+})cpp");
+  FS.Files[FooCpp] = FooCpp;
+
+  auto Opts = clangd::CodeCompleteOptions();
+  Opts.AllowFallbackMode = true;
+
+  // This will make compile command broken and preamble absent.
+  CDB.ExtraClangFlags = {"yolo.cc"};
+  Server.addDocument(FooCpp, Code.code());
+  ASSERT_TRUE(Server.blockUntilIdleForTest());
+  auto Res = cantFail(runCodeComplete(Server, FooCpp, Code.point(), Opts));
+  EXPECT_THAT(Res.Completions, IsEmpty());
+  EXPECT_EQ(Res.Context, CodeCompletionContext::CCC_Recovery);
+
+  // Make the compile command work again.
+  CDB.ExtraClangFlags = {"-std=c++11"};
+  Server.addDocument(FooCpp, Code.code());
+  ASSERT_TRUE(Server.blockUntilIdleForTest());
+  EXPECT_THAT(cantFail(runCodeComplete(Server, FooCpp, Code.point(),
+   Opts))
+  .Completions,
+  ElementsAre(Field(::Name, "xyz")));
+}
+
 } // namespace
 } // namespace clangd
 } // namespace clang
Index: clangd/tool/ClangdMain.cpp
===
--- clangd/tool/ClangdMain.cpp
+++ clangd/tool/ClangdMain.cpp
@@ -231,6 +231,14 @@
 "Offsets are in UTF-16 code units")),
 llvm::cl::init(OffsetEncoding::UnsupportedEncoding));
 
+static llvm::cl::opt AllowFallbackCompletion(
+"allow-fallback-completion",
+llvm::cl::desc(
+"Allow falling back to code completion without compiling files (using "
+"identifiers and symbol indexes), when file cannot be built or the "
+"build is not ready."),
+llvm::cl::init(false));
+
 namespace {
 
 /// \brief Supports a test URI scheme with relaxed constraints for lit tests.
@@ -437,6 +445,7 @@
   CCOpts.SpeculativeIndexRequest = Opts.StaticIndex;
   CCOpts.EnableFunctionArgSnippets = EnableFunctionArgSnippets;
   CCOpts.AllScopes = AllScopesCompletion;
+  CCOpts.AllowFallbackMode = AllowFallbackCompletion;
 
   RealFileSystemProvider FSProvider;
   // Initialize and run ClangdLSPServer.
Index: clangd/Threading.h
===
--- clangd/Threading.h
+++ clangd/Threading.h
@@ -27,6 +27,7 @@
 public:
   // Sets the flag. No-op if already set.
   void notify();
+  bool notified() const { return Notified; }
   // Blocks until flag is set.
   void wait() const;
 
Index: clangd/TUScheduler.h
===
--- clangd/TUScheduler.h
+++ clangd/TUScheduler.h
@@ -13,7 +13,9 @@
 #include "Function.h"
 #include "Threading.h"
 #include "index/CanonicalIncludes.h"
+#include "llvm/ADT/Optional.h"
 #include "llvm/ADT/StringMap.h"
+#include "llvm/ADT/StringRef.h"
 #include 
 
 namespace clang {
@@ -32,6 +34,7 @@
 struct InputsAndPreamble {
   llvm::StringRef Contents;
   const tooling::CompileCommand 
+  // This can be nullptr if no preamble is availble.
   const PreambleData *Preamble;
 };
 
@@ -178,13 +181,19 @@
 ///   reading source code from headers.
 /// This is the fastest option, usually a preamble is available immediately.
 Stale,
+/// Besides accepting stale preamble, this also allow preamble to be absent
+/// (not ready or failed to build).
+StaleOrAbsent,
   };
+
   /// Schedule an async read of the preamble.
-  /// If there's no preamble yet (because the file was just opened), we'll wait
-  /// for it to build. The result may be null if it fails to build or is empty.
-  /// If an error occurs, it is forwarded to the \p Action callback.
-  /// Context cancellation 

[PATCH] D59811: [clangd] Add fallback mode for code completion when compile command or preamble is not ready.

[Eric Liu via Phabricator via cfe-commits]

ioeric planned changes to this revision.
ioeric added a comment.

Oops, forgot to update tests...


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D59811



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

[PATCH] D59811: [clangd] Add fallback mode for code completion when compile command or preamble is not ready.

[Eric Liu via Phabricator via cfe-commits]

ioeric added inline comments.



Comment at: clangd/TUScheduler.h:204
+   Callback Action,
+   bool AllowFallback = false);
 

ilya-biryukov wrote:
> sammccall wrote:
> > I think this isn't orthogonal to `PreambleConsistency`.
> > When would we use AllowFallback = true but PreambleConsistency = Consistent?
> > 
> > Two possible options:
> >  - adding a new `StaleOrAbsent` option to PreambleConsistency
> >  - changing `Stale` to these new semantics, as codeComplete is the only 
> > caller
> > The problem with the latter is we can't put it behind a flag.
> Ah, I was totally looking past the `PreambleConsistency` flag.
> Thanks for spotting this. Indeed, modeling the fallback in the 
> `PreambleConsistency` would make sense.
Sounds good. Thanks for the suggestion!


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D59811



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

[PATCH] D59811: [clangd] Add fallback mode for code completion when compile command or preamble is not ready.

[Eric Liu via Phabricator via cfe-commits]

ioeric updated this revision to Diff 194129.
ioeric marked 3 inline comments as done.
ioeric added a comment.

- split out the compile command change.


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D59811

Files:
  clangd/ClangdServer.cpp
  clangd/CodeComplete.h
  clangd/TUScheduler.cpp
  clangd/TUScheduler.h
  clangd/Threading.h
  clangd/tool/ClangdMain.cpp
  unittests/clangd/ClangdTests.cpp

Index: unittests/clangd/ClangdTests.cpp
===
--- unittests/clangd/ClangdTests.cpp
+++ unittests/clangd/ClangdTests.cpp
@@ -13,8 +13,10 @@
 #include "Matchers.h"
 #include "SyncAPI.h"
 #include "TestFS.h"
+#include "Threading.h"
 #include "URI.h"
 #include "clang/Config/config.h"
+#include "clang/Sema/CodeCompleteConsumer.h"
 #include "llvm/ADT/SmallVector.h"
 #include "llvm/ADT/StringMap.h"
 #include "llvm/Support/Errc.h"
@@ -36,7 +38,6 @@
 namespace {
 
 using ::testing::ElementsAre;
-using ::testing::Eq;
 using ::testing::Field;
 using ::testing::Gt;
 using ::testing::IsEmpty;
@@ -1058,6 +1059,33 @@
   EXPECT_NE(Result, "");
 }
 
+TEST_F(ClangdVFSTest, FallbackWhenPreambleIsNotReady) {
+  MockFSProvider FS;
+  ErrorCheckingDiagConsumer DiagConsumer;
+  MockCompilationDatabase CDB;
+  ClangdServer Server(CDB, FS, DiagConsumer, ClangdServer::optsForTest());
+
+  // XXX provide a broken command first and then a good command.
+  auto FooCpp = testPath("foo.cpp");
+  Annotations Code(R"cpp(
+int main() {
+  int xyz;
+  xy^
+})cpp");
+  FS.Files[FooCpp] = FooCpp;
+  Server.addDocument(FooCpp, Code.code());
+  auto Opts = clangd::CodeCompleteOptions();
+  Opts.AllowFallbackMode = true;
+  auto Res = cantFail(runCodeComplete(Server, FooCpp, Code.point(), Opts));
+  EXPECT_THAT(Res.Completions, IsEmpty());
+  EXPECT_EQ(Res.Context, CodeCompletionContext::CCC_Recovery);
+  ASSERT_TRUE(Server.blockUntilIdleForTest());
+  EXPECT_THAT(cantFail(runCodeComplete(Server, FooCpp, Code.point(),
+   clangd::CodeCompleteOptions()))
+  .Completions,
+  ElementsAre(Field(::Name, "xyz")));
+}
+
 } // namespace
 } // namespace clangd
 } // namespace clang
Index: clangd/tool/ClangdMain.cpp
===
--- clangd/tool/ClangdMain.cpp
+++ clangd/tool/ClangdMain.cpp
@@ -231,6 +231,14 @@
 "Offsets are in UTF-16 code units")),
 llvm::cl::init(OffsetEncoding::UnsupportedEncoding));
 
+static llvm::cl::opt AllowFallbackCompletion(
+"allow-fallback-completion",
+llvm::cl::desc(
+"Allow falling back to code completion without compiling files (using "
+"identifiers and symbol indexes), when file cannot be built or the "
+"build is not ready."),
+llvm::cl::init(false));
+
 namespace {
 
 /// \brief Supports a test URI scheme with relaxed constraints for lit tests.
@@ -437,6 +445,7 @@
   CCOpts.SpeculativeIndexRequest = Opts.StaticIndex;
   CCOpts.EnableFunctionArgSnippets = EnableFunctionArgSnippets;
   CCOpts.AllScopes = AllScopesCompletion;
+  CCOpts.AllowFallbackMode = AllowFallbackCompletion;
 
   RealFileSystemProvider FSProvider;
   // Initialize and run ClangdLSPServer.
Index: clangd/Threading.h
===
--- clangd/Threading.h
+++ clangd/Threading.h
@@ -27,6 +27,7 @@
 public:
   // Sets the flag. No-op if already set.
   void notify();
+  bool notified() const { return Notified; }
   // Blocks until flag is set.
   void wait() const;
 
Index: clangd/TUScheduler.h
===
--- clangd/TUScheduler.h
+++ clangd/TUScheduler.h
@@ -13,7 +13,9 @@
 #include "Function.h"
 #include "Threading.h"
 #include "index/CanonicalIncludes.h"
+#include "llvm/ADT/Optional.h"
 #include "llvm/ADT/StringMap.h"
+#include "llvm/ADT/StringRef.h"
 #include 
 
 namespace clang {
@@ -32,6 +34,7 @@
 struct InputsAndPreamble {
   llvm::StringRef Contents;
   const tooling::CompileCommand 
+  // This can be nullptr if no preamble is availble.
   const PreambleData *Preamble;
 };
 
@@ -178,13 +181,19 @@
 ///   reading source code from headers.
 /// This is the fastest option, usually a preamble is available immediately.
 Stale,
+/// Besides accepting stale preamble, this also allow preamble to be absent
+/// (not ready or failed to build).
+StaleOrAbsent,
   };
+
   /// Schedule an async read of the preamble.
-  /// If there's no preamble yet (because the file was just opened), we'll wait
-  /// for it to build. The result may be null if it fails to build or is empty.
-  /// If an error occurs, it is forwarded to the \p Action callback.
-  /// Context cancellation is ignored and should be handled by the Action.
-  /// (In practice, the Action is almost always executed immediately).
+  /// If 

[PATCH] D60316: [clangd] Include insertion: require header guards, drop other heuristics, treat .def like .inc.

[Eric Liu via Phabricator via cfe-commits]

ioeric added inline comments.



Comment at: clangd/index/SymbolCollector.cpp:157
+  return Canonical.str();
+else if (Canonical != Filename)
+  return toURI(SM, Canonical, Opts);

nit: no need for `else`?



Comment at: unittests/clangd/FileIndexTests.cpp:214
 
 TEST(FileIndexTest, HasSystemHeaderMappingsInPreamble) {
+  TestTU TU;

sammccall wrote:
> I suspect we can replace most of these tests with TestTU - here I just 
> changed the ones where the include guard would otherwise be needed.
This looks good to me.

I think this test case tried to explicitly test that preamble callback has the 
expected `CanonicalIncludes`. Using `TestTU` seems to achieve the same goal but 
makes the intention a bit less clear though. 



Comment at: unittests/clangd/TestTU.h:55
 
+  // Simulate a header guard of the header (using an #import directive).
+  bool ImplicitHeaderGuard = true;

is this used?


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D60316



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

[PATCH] D59376: [LibTooling] Add Transformer, a library for source-to-source transformations.

[Eric Liu via Phabricator via cfe-commits]

ioeric added a comment.

In D59376#1454834 , @ymandel wrote:

> In D59376#1454768 , @ilya-biryukov 
> wrote:
>
> > Per @ioeric's suggestion: why not move the helper into 
> > `Tooling/Refactoring/ExtendedRange.h`?
> >  If it's in `ToolingRefactoring`, both stencil and transformer can access 
> > it.
> >
> > For external users, a dependency on either `ToolingCore` or 
> > `ToolingRefactoring` should be fine, since they're fine with a dependency 
> > on `Tooling` already.
>
>
> This sounds perfect. Can I do the same for the future additions -- small, 
> focused libraries for each group of functions? I just want to be sure that we 
> don't regret the name "ExtendedRange" when I need to add the next batch.


In clangd, we have a library called `SourceCode.h` that keeps source code 
related helpers including those that deal with ranges. I think it's reasonable 
to use SourceCode.h or something similar here.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D59376



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

[PATCH] D60263: [clang-format] Preserve include blocks in ObjC Google style

[Eric Liu via Phabricator via cfe-commits]

ioeric added inline comments.



Comment at: lib/Format/Format.cpp:787
   GoogleStyle.IncludeStyle.IncludeIsMainRegex = "([-_](test|unittest))?$";
   GoogleStyle.IncludeStyle.IncludeBlocks = tooling::IncludeStyle::IBS_Regroup;
   GoogleStyle.IndentCaseLabels = true;

maybe we should also only use regroup for cpp? `regroup` is only supported in 
`sortCppIncludes` after all.


Repository:
  rC Clang

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

https://reviews.llvm.org/D60263



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

[PATCH] D60257: [Lookup] Invisible decls should not be ambiguous when renaming.

[Eric Liu via Phabricator via cfe-commits]

ioeric created this revision.
ioeric added a reviewer: hokein.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

For example, a renamed type in a header file can conflict with declaration in
a random file that includes the header, but we should not consider the decl 
ambiguous if
it's not visible at the rename location. This improves consistency of generated 
replacements
when header file is included in different TUs.


Repository:
  rC Clang

https://reviews.llvm.org/D60257

Files:
  include/clang/Tooling/Core/Lookup.h
  lib/Tooling/Core/Lookup.cpp
  lib/Tooling/Refactoring/Rename/USRLocFinder.cpp
  unittests/Tooling/LookupTest.cpp

Index: unittests/Tooling/LookupTest.cpp
===
--- unittests/Tooling/LookupTest.cpp
+++ unittests/Tooling/LookupTest.cpp
@@ -44,8 +44,8 @@
 const auto *Callee = cast(Expr->getCallee()->IgnoreImplicit());
 const ValueDecl *FD = Callee->getDecl();
 return tooling::replaceNestedName(
-Callee->getQualifier(), Visitor.DeclStack.back()->getDeclContext(), FD,
-ReplacementString);
+Callee->getQualifier(), Callee->getLocation(),
+Visitor.DeclStack.back()->getDeclContext(), FD, ReplacementString);
   };
 
   Visitor.OnCall = [&](CallExpr *Expr) {
@@ -181,12 +181,12 @@
 TEST(LookupTest, replaceNestedClassName) {
   GetDeclsVisitor Visitor;
 
-  auto replaceRecordTypeLoc = [&](RecordTypeLoc Loc,
+  auto replaceRecordTypeLoc = [&](RecordTypeLoc TLoc,
   StringRef ReplacementString) {
-const auto *FD = cast(Loc.getDecl());
+const auto *FD = cast(TLoc.getDecl());
 return tooling::replaceNestedName(
-nullptr, Visitor.DeclStack.back()->getDeclContext(), FD,
-ReplacementString);
+nullptr, TLoc.getBeginLoc(), Visitor.DeclStack.back()->getDeclContext(),
+FD, ReplacementString);
   };
 
   Visitor.OnRecordTypeLoc = [&](RecordTypeLoc Type) {
@@ -211,6 +211,36 @@
   };
   Visitor.runOver("namespace a { namespace b { class Foo {}; } }\n"
   "namespace c { using a::b::Foo; Foo f();; }\n");
+
+  // Potentially ambiguous symbols that are not visible at reference location
+  // are not considered ambiguous.
+  Visitor.OnRecordTypeLoc = [&](RecordTypeLoc Type) {
+if (Type.getDecl()->getQualifiedNameAsString() == "x::y::Old")
+  EXPECT_EQ("Foo", replaceRecordTypeLoc(Type, "::x::Foo"));
+  };
+  Visitor.runOver(R"(
+// Definitions.
+namespace x {
+ namespace y {
+  class Old {};
+  class Other {};
+ }
+}
+
+// Suppose this is a reference in some header file.
+namespace x {
+ namespace y {
+  Old f;  // This is to be renamed.
+ }
+}
+
+// Suppose this is in a file that includes the above header.
+namespace x {
+namespace y {
+ using Foo = ::x::y::Other;
+}
+}
+)");
 }
 
 } // end anonymous namespace
Index: lib/Tooling/Refactoring/Rename/USRLocFinder.cpp
===
--- lib/Tooling/Refactoring/Rename/USRLocFinder.cpp
+++ lib/Tooling/Refactoring/Rename/USRLocFinder.cpp
@@ -542,8 +542,8 @@
 if (!llvm::isa(
 RenameInfo.Context->getDeclContext())) {
   ReplacedName = tooling::replaceNestedName(
-  RenameInfo.Specifier, RenameInfo.Context->getDeclContext(),
-  RenameInfo.FromDecl,
+  RenameInfo.Specifier, RenameInfo.Begin,
+  RenameInfo.Context->getDeclContext(), RenameInfo.FromDecl,
   NewName.startswith("::") ? NewName.str()
: ("::" + NewName).str());
 } else {
Index: lib/Tooling/Core/Lookup.cpp
===
--- lib/Tooling/Core/Lookup.cpp
+++ lib/Tooling/Core/Lookup.cpp
@@ -14,6 +14,7 @@
 #include "clang/AST/Decl.h"
 #include "clang/AST/DeclCXX.h"
 #include "clang/AST/DeclarationName.h"
+#include "clang/Basic/SourceLocation.h"
 #include "llvm/ADT/SmallVector.h"
 using namespace clang;
 using namespace clang::tooling;
@@ -123,7 +124,8 @@
 // FIXME: consider using namespaces.
 static std::string disambiguateSpellingInScope(StringRef Spelling,
StringRef QName,
-   const DeclContext ) {
+   const DeclContext ,
+   SourceLocation UseLoc) {
   assert(QName.startswith("::"));
   assert(QName.endswith(Spelling));
   if (Spelling.startswith("::"))
@@ -138,9 +140,10 @@
   getAllNamedNamespaces();
   auto  = UseContext.getParentASTContext();
   StringRef TrimmedQName = QName.substr(2);
+  const auto  = UseContext.getParentASTContext().getSourceManager();
+  UseLoc = SM.getSpellingLoc(UseLoc);
 
-  auto IsAmbiguousSpelling = [, , ](
- const llvm::StringRef 

[PATCH] D60199: [clang-format] Do not emit replacements while regrouping if Cpp includes are OK

[Eric Liu via Phabricator via cfe-commits]

ioeric added inline comments.



Comment at: unittests/Format/SortIncludesTest.cpp:28
   std::string sort(StringRef Code, std::vector Ranges,
+   llvm::Optional ExpectedNumRanges = llvm::None,
StringRef FileName = "input.cc") {

As most cases would pass `1`, maybe it would make sense to make this a class 
member that defaults to `1`? It can be explicitly set in specific tests when 
needed.


Repository:
  rC Clang

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

https://reviews.llvm.org/D60199



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

[PATCH] D60116: [clang-format] Regroup #includes into blocks for Google style

[Eric Liu via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rC357567: [clang-format] Regroup #includes into blocks for 
Google style (authored by ioeric, committed by ).

Changed prior to commit:
  https://reviews.llvm.org/D60116?vs=193446=193452#toc

Repository:
  rC Clang

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

https://reviews.llvm.org/D60116

Files:
  lib/Format/Format.cpp
  unittests/Format/CleanupTest.cpp
  unittests/Format/SortIncludesTest.cpp


Index: lib/Format/Format.cpp
===
--- lib/Format/Format.cpp
+++ lib/Format/Format.cpp
@@ -784,6 +784,7 @@
   GoogleStyle.IncludeStyle.IncludeCategories = {
   {"^", 2}, {"^<.*\\.h>", 1}, {"^<.*", 2}, {".*", 3}};
   GoogleStyle.IncludeStyle.IncludeIsMainRegex = "([-_](test|unittest))?$";
+  GoogleStyle.IncludeStyle.IncludeBlocks = tooling::IncludeStyle::IBS_Regroup;
   GoogleStyle.IndentCaseLabels = true;
   GoogleStyle.KeepEmptyLinesAtTheStartOfBlocks = false;
   GoogleStyle.ObjCBinPackProtocolList = FormatStyle::BPS_Never;
Index: unittests/Format/CleanupTest.cpp
===
--- unittests/Format/CleanupTest.cpp
+++ unittests/Format/CleanupTest.cpp
@@ -420,8 +420,10 @@
 TEST_F(CleanUpReplacementsTest, InsertMultipleNewHeadersAndSortGoogle) {
   std::string Code = "\nint x;";
   std::string Expected = "\n#include \"fix.h\"\n"
+ "\n"
  "#include \n"
  "#include \n"
+ "\n"
  "#include \"a.h\"\n"
  "#include \"b.h\"\n"
  "#include \"c.h\"\n"
Index: unittests/Format/SortIncludesTest.cpp
===
--- unittests/Format/SortIncludesTest.cpp
+++ unittests/Format/SortIncludesTest.cpp
@@ -262,9 +262,13 @@
 TEST_F(SortIncludesTest, HandlesAngledIncludesAsSeparateBlocks) {
   EXPECT_EQ("#include \"a.h\"\n"
 "#include \"c.h\"\n"
+"#include \n"
 "#include \n"
-"#include \n",
-sort("#include \n"
+"#include \n"
+"#include \n",
+sort("#include \n"
+ "#include \n"
+ "#include \n"
  "#include \n"
  "#include \"c.h\"\n"
  "#include \"a.h\"\n"));
@@ -272,9 +276,15 @@
   FmtStyle = getGoogleStyle(FormatStyle::LK_Cpp);
   EXPECT_EQ("#include \n"
 "#include \n"
+"\n"
+"#include \n"
+"#include \n"
+"\n"
 "#include \"a.h\"\n"
 "#include \"c.h\"\n",
-sort("#include \n"
+sort("#include \n"
+ "#include \n"
+ "#include \n"
  "#include \n"
  "#include \"c.h\"\n"
  "#include \"a.h\"\n"));


Index: lib/Format/Format.cpp
===
--- lib/Format/Format.cpp
+++ lib/Format/Format.cpp
@@ -784,6 +784,7 @@
   GoogleStyle.IncludeStyle.IncludeCategories = {
   {"^", 2}, {"^<.*\\.h>", 1}, {"^<.*", 2}, {".*", 3}};
   GoogleStyle.IncludeStyle.IncludeIsMainRegex = "([-_](test|unittest))?$";
+  GoogleStyle.IncludeStyle.IncludeBlocks = tooling::IncludeStyle::IBS_Regroup;
   GoogleStyle.IndentCaseLabels = true;
   GoogleStyle.KeepEmptyLinesAtTheStartOfBlocks = false;
   GoogleStyle.ObjCBinPackProtocolList = FormatStyle::BPS_Never;
Index: unittests/Format/CleanupTest.cpp
===
--- unittests/Format/CleanupTest.cpp
+++ unittests/Format/CleanupTest.cpp
@@ -420,8 +420,10 @@
 TEST_F(CleanUpReplacementsTest, InsertMultipleNewHeadersAndSortGoogle) {
   std::string Code = "\nint x;";
   std::string Expected = "\n#include \"fix.h\"\n"
+ "\n"
  "#include \n"
  "#include \n"
+ "\n"
  "#include \"a.h\"\n"
  "#include \"b.h\"\n"
  "#include \"c.h\"\n"
Index: unittests/Format/SortIncludesTest.cpp
===
--- unittests/Format/SortIncludesTest.cpp
+++ unittests/Format/SortIncludesTest.cpp
@@ -262,9 +262,13 @@
 TEST_F(SortIncludesTest, HandlesAngledIncludesAsSeparateBlocks) {
   EXPECT_EQ("#include \"a.h\"\n"
 "#include \"c.h\"\n"
+"#include \n"
 "#include \n"
-"#include \n",
-sort("#include \n"
+"#include \n"
+"#include \n",
+sort("#include \n"
+ "#include \n"
+ "#include \n"
  "#include \n"
  "#include \"c.h\"\n"
  "#include \"a.h\"\n"));
@@ -272,9 

[PATCH] D60116: [clang-format] Regroup #includes into blocks for Google style

[Eric Liu via Phabricator via cfe-commits]

ioeric updated this revision to Diff 193446.
ioeric added a comment.

- Improved test.


Repository:
  rC Clang

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

https://reviews.llvm.org/D60116

Files:
  lib/Format/Format.cpp
  unittests/Format/CleanupTest.cpp
  unittests/Format/SortIncludesTest.cpp


Index: unittests/Format/SortIncludesTest.cpp
===
--- unittests/Format/SortIncludesTest.cpp
+++ unittests/Format/SortIncludesTest.cpp
@@ -262,9 +262,13 @@
 TEST_F(SortIncludesTest, HandlesAngledIncludesAsSeparateBlocks) {
   EXPECT_EQ("#include \"a.h\"\n"
 "#include \"c.h\"\n"
+"#include \n"
 "#include \n"
-"#include \n",
-sort("#include \n"
+"#include \n"
+"#include \n",
+sort("#include \n"
+ "#include \n"
+ "#include \n"
  "#include \n"
  "#include \"c.h\"\n"
  "#include \"a.h\"\n"));
@@ -272,9 +276,15 @@
   FmtStyle = getGoogleStyle(FormatStyle::LK_Cpp);
   EXPECT_EQ("#include \n"
 "#include \n"
+"\n"
+"#include \n"
+"#include \n"
+"\n"
 "#include \"a.h\"\n"
 "#include \"c.h\"\n",
-sort("#include \n"
+sort("#include \n"
+ "#include \n"
+ "#include \n"
  "#include \n"
  "#include \"c.h\"\n"
  "#include \"a.h\"\n"));
Index: unittests/Format/CleanupTest.cpp
===
--- unittests/Format/CleanupTest.cpp
+++ unittests/Format/CleanupTest.cpp
@@ -420,8 +420,10 @@
 TEST_F(CleanUpReplacementsTest, InsertMultipleNewHeadersAndSortGoogle) {
   std::string Code = "\nint x;";
   std::string Expected = "\n#include \"fix.h\"\n"
+ "\n"
  "#include \n"
  "#include \n"
+ "\n"
  "#include \"a.h\"\n"
  "#include \"b.h\"\n"
  "#include \"c.h\"\n"
Index: lib/Format/Format.cpp
===
--- lib/Format/Format.cpp
+++ lib/Format/Format.cpp
@@ -784,6 +784,7 @@
   GoogleStyle.IncludeStyle.IncludeCategories = {
   {"^", 2}, {"^<.*\\.h>", 1}, {"^<.*", 2}, {".*", 3}};
   GoogleStyle.IncludeStyle.IncludeIsMainRegex = "([-_](test|unittest))?$";
+  GoogleStyle.IncludeStyle.IncludeBlocks = tooling::IncludeStyle::IBS_Regroup;
   GoogleStyle.IndentCaseLabels = true;
   GoogleStyle.KeepEmptyLinesAtTheStartOfBlocks = false;
   GoogleStyle.ObjCBinPackProtocolList = FormatStyle::BPS_Never;


Index: unittests/Format/SortIncludesTest.cpp
===
--- unittests/Format/SortIncludesTest.cpp
+++ unittests/Format/SortIncludesTest.cpp
@@ -262,9 +262,13 @@
 TEST_F(SortIncludesTest, HandlesAngledIncludesAsSeparateBlocks) {
   EXPECT_EQ("#include \"a.h\"\n"
 "#include \"c.h\"\n"
+"#include \n"
 "#include \n"
-"#include \n",
-sort("#include \n"
+"#include \n"
+"#include \n",
+sort("#include \n"
+ "#include \n"
+ "#include \n"
  "#include \n"
  "#include \"c.h\"\n"
  "#include \"a.h\"\n"));
@@ -272,9 +276,15 @@
   FmtStyle = getGoogleStyle(FormatStyle::LK_Cpp);
   EXPECT_EQ("#include \n"
 "#include \n"
+"\n"
+"#include \n"
+"#include \n"
+"\n"
 "#include \"a.h\"\n"
 "#include \"c.h\"\n",
-sort("#include \n"
+sort("#include \n"
+ "#include \n"
+ "#include \n"
  "#include \n"
  "#include \"c.h\"\n"
  "#include \"a.h\"\n"));
Index: unittests/Format/CleanupTest.cpp
===
--- unittests/Format/CleanupTest.cpp
+++ unittests/Format/CleanupTest.cpp
@@ -420,8 +420,10 @@
 TEST_F(CleanUpReplacementsTest, InsertMultipleNewHeadersAndSortGoogle) {
   std::string Code = "\nint x;";
   std::string Expected = "\n#include \"fix.h\"\n"
+ "\n"
  "#include \n"
  "#include \n"
+ "\n"
  "#include \"a.h\"\n"
  "#include \"b.h\"\n"
  "#include \"c.h\"\n"
Index: lib/Format/Format.cpp
===
--- lib/Format/Format.cpp
+++ lib/Format/Format.cpp
@@ -784,6 +784,7 @@
   GoogleStyle.IncludeStyle.IncludeCategories = {
   {"^", 2}, {"^<.*\\.h>", 1}, {"^<.*", 2}, {".*", 

[PATCH] D60126: [clangd] Use identifiers in file as completion candidates when build is not ready.

[Eric Liu via Phabricator via cfe-commits]

ioeric created this revision.
ioeric added a reviewer: sammccall.
Herald added subscribers: cfe-commits, jdoerfert, kadircet, arphaman, jkorous, 
MaskRay, ilya-biryukov.
Herald added a project: clang.

o Lex the code to get the identifiers and put them into a "symbol" index.
o Adds a new completion mode without compilation/sema into code completion 
workflow.
o Make IncludeInserter work even when no compile command is present, by avoiding
inserting non-verbatim headers.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D60126

Files:
  clangd/ClangdServer.cpp
  clangd/ClangdUnit.cpp
  clangd/CodeComplete.cpp
  clangd/CodeComplete.h
  clangd/Headers.cpp
  clangd/Headers.h
  unittests/clangd/ClangdTests.cpp
  unittests/clangd/CodeCompleteTests.cpp
  unittests/clangd/HeadersTests.cpp

Index: unittests/clangd/HeadersTests.cpp
===
--- unittests/clangd/HeadersTests.cpp
+++ unittests/clangd/HeadersTests.cpp
@@ -90,7 +90,7 @@
 
 IncludeInserter Inserter(MainFile, /*Code=*/"", format::getLLVMStyle(),
  CDB.getCompileCommand(MainFile)->Directory,
- Clang->getPreprocessor().getHeaderSearchInfo());
+ >getPreprocessor().getHeaderSearchInfo());
 for (const auto  : Inclusions)
   Inserter.addExisting(Inc);
 auto Declaring = ToHeaderFile(Original);
@@ -110,7 +110,7 @@
 
 IncludeInserter Inserter(MainFile, /*Code=*/"", format::getLLVMStyle(),
  CDB.getCompileCommand(MainFile)->Directory,
- Clang->getPreprocessor().getHeaderSearchInfo());
+ >getPreprocessor().getHeaderSearchInfo());
 auto Edit = Inserter.insert(VerbatimHeader);
 Action.EndSourceFile();
 return Edit;
@@ -252,6 +252,24 @@
   EXPECT_TRUE(StringRef(Edit->newText).contains(""));
 }
 
+TEST(Headers, NoHeaderSearchInfo) {
+  std::string MainFile = testPath("main.cpp");
+  IncludeInserter Inserter(MainFile, /*Code=*/"", format::getLLVMStyle(),
+   /*BuildDir=*/"", /*HeaderSearchInfo=*/nullptr);
+
+  auto HeaderPath = testPath("sub/bar.h");
+  auto Declaring = HeaderFile{HeaderPath, /*Verbatim=*/false};
+  auto Inserting = HeaderFile{HeaderPath, /*Verbatim=*/false};
+  auto Verbatim = HeaderFile{"", /*Verbatim=*/true};
+
+  EXPECT_EQ(Inserter.calculateIncludePath(Declaring, Inserting),
+"\"" + HeaderPath + "\"");
+  EXPECT_EQ(Inserter.shouldInsertInclude(Declaring, Inserting), false);
+
+  EXPECT_EQ(Inserter.calculateIncludePath(Declaring, Verbatim), "");
+  EXPECT_EQ(Inserter.shouldInsertInclude(Declaring, Verbatim), true);
+}
+
 } // namespace
 } // namespace clangd
 } // namespace clang
Index: unittests/clangd/CodeCompleteTests.cpp
===
--- unittests/clangd/CodeCompleteTests.cpp
+++ unittests/clangd/CodeCompleteTests.cpp
@@ -32,7 +32,6 @@
 using ::llvm::Failed;
 using ::testing::AllOf;
 using ::testing::Contains;
-using ::testing::Each;
 using ::testing::ElementsAre;
 using ::testing::Field;
 using ::testing::HasSubstr;
@@ -139,6 +138,25 @@
  FilePath);
 }
 
+// Builds a server and runs code completion.
+// If IndexSymbols is non-empty, an index will be built and passed to opts.
+CodeCompleteResult completionsNoCompile(llvm::StringRef Text,
+std::vector IndexSymbols = {},
+clangd::CodeCompleteOptions Opts = {},
+PathRef FilePath = "foo.cpp") {
+  std::unique_ptr OverrideIndex;
+  if (!IndexSymbols.empty()) {
+assert(!Opts.Index && "both Index and IndexSymbols given!");
+OverrideIndex = memIndex(std::move(IndexSymbols));
+Opts.Index = OverrideIndex.get();
+  }
+
+  MockFSProvider FS;
+  Annotations Test(Text);
+  return codeCompleteNoCompile(FilePath, Test.code(), Test.point(),
+   FS.getFileSystem(), Opts);
+}
+
 Symbol withReferences(int N, Symbol S) {
   S.References = N;
   return S;
@@ -2366,6 +2384,33 @@
   UnorderedElementsAre(AllOf(Qualifier(""), Named("XYZ";
 }
 
+TEST(NoCompileCompletionTest, Basic) {
+  auto Results = completionsNoCompile(R"cpp(
+void func() {
+  int xyz;
+  int abc;
+  ^
+}
+  )cpp");
+  EXPECT_THAT(Results.Completions,
+  UnorderedElementsAre(Named("void"), Named("func"), Named("int"),
+   Named("xyz"), Named("abc")));
+}
+
+TEST(NoCompileCompletionTest, WithFilter) {
+  auto Results = completionsNoCompile(R"cpp(
+void func() {
+  int sym1;
+  int sym2;
+  int xyz1;
+  int xyz2;
+  sy^
+}
+  )cpp");
+  EXPECT_THAT(Results.Completions,
+  UnorderedElementsAre(Named("sym1"), Named("sym2")));
+}
+
 } // namespace
 } // namespace clangd
 } // namespace clang
Index: 

[PATCH] D59811: [clangd] Add fallback mode for code completion when compile command or preamble is not ready.

[Eric Liu via Phabricator via cfe-commits]

ioeric updated this revision to Diff 193268.
ioeric added a comment.

- Rebase


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D59811

Files:
  clangd/ClangdServer.cpp
  clangd/CodeComplete.h
  clangd/TUScheduler.cpp
  clangd/TUScheduler.h
  clangd/Threading.h
  clangd/tool/ClangdMain.cpp
  unittests/clangd/ClangdTests.cpp
  unittests/clangd/TUSchedulerTests.cpp

Index: unittests/clangd/TUSchedulerTests.cpp
===
--- unittests/clangd/TUSchedulerTests.cpp
+++ unittests/clangd/TUSchedulerTests.cpp
@@ -15,14 +15,13 @@
 #include "gmock/gmock.h"
 #include "gtest/gtest.h"
 #include 
+#include 
 #include 
 
 namespace clang {
 namespace clangd {
 namespace {
 
-using ::testing::_;
-using ::testing::AllOf;
 using ::testing::AnyOf;
 using ::testing::Each;
 using ::testing::ElementsAre;
@@ -35,13 +34,18 @@
 
 class TUSchedulerTests : public ::testing::Test {
 protected:
-  ParseInputs getInputs(PathRef File, std::string Contents) {
+  FileUpdateInputs getInputs(PathRef File, std::string Contents) {
 ParseInputs Inputs;
-Inputs.CompileCommand = *CDB.getCompileCommand(File);
 Inputs.FS = buildTestFS(Files, Timestamps);
 Inputs.Contents = std::move(Contents);
 Inputs.Opts = ParseOptions();
-return Inputs;
+FileUpdateInputs UpdateInputs;
+UpdateInputs.InputSansCommand = std::move(Inputs);
+std::string FilePath = File;
+UpdateInputs.GetCompileCommand = [this, FilePath]() {
+  return *CDB.getCompileCommand(FilePath);
+};
+return UpdateInputs;
   }
 
   void updateWithCallback(TUScheduler , PathRef File,
@@ -72,7 +76,7 @@
   /// Schedule an update and call \p CB with the diagnostics it produces, if
   /// any. The TUScheduler should be created with captureDiags as a
   /// DiagsCallback for this to work.
-  void updateWithDiags(TUScheduler , PathRef File, ParseInputs Inputs,
+  void updateWithDiags(TUScheduler , PathRef File, FileUpdateInputs Inputs,
WantDiagnostics WD,
llvm::unique_function)> CB) {
 Path OrigFile = File.str();
@@ -245,7 +249,7 @@
 
 S.runWithPreamble("StaleRead", Path, TUScheduler::Stale,
   [&](Expected Pre) {
-ASSERT_TRUE(bool(Pre));
+EXPECT_TRUE((bool)Pre);
 assert(bool(Pre));
 EXPECT_THAT(includes(Pre->Preamble),
 ElementsAre(""));
@@ -397,8 +401,9 @@
 EXPECT_THAT(Context::current().get(NonceKey), Pointee(Nonce));
 
 ASSERT_TRUE((bool)AST);
-EXPECT_EQ(AST->Inputs.FS, Inputs.FS);
-EXPECT_EQ(AST->Inputs.Contents, Inputs.Contents);
+EXPECT_EQ(AST->Inputs.FS, Inputs.InputSansCommand.FS);
+EXPECT_EQ(AST->Inputs.Contents,
+  Inputs.InputSansCommand.Contents);
 
 std::lock_guard Lock(Mut);
 ++TotalASTReads;
@@ -415,7 +420,7 @@
 EXPECT_THAT(Context::current().get(NonceKey), Pointee(Nonce));
 
 ASSERT_TRUE((bool)Preamble);
-EXPECT_EQ(Preamble->Contents, Inputs.Contents);
+EXPECT_EQ(Preamble->Contents, Inputs.InputSansCommand.Contents);
 
 std::lock_guard Lock(Mut);
 ++TotalPreambleReads;
@@ -504,6 +509,7 @@
   )cpp";
   auto WithEmptyPreamble = R"cpp(int main() {})cpp";
   S.update(Foo, getInputs(Foo, WithPreamble), WantDiagnostics::Auto);
+
   S.runWithPreamble(
   "getNonEmptyPreamble", Foo, TUScheduler::Stale,
   [&](Expected Preamble) {
Index: unittests/clangd/ClangdTests.cpp
===
--- unittests/clangd/ClangdTests.cpp
+++ unittests/clangd/ClangdTests.cpp
@@ -13,8 +13,10 @@
 #include "Matchers.h"
 #include "SyncAPI.h"
 #include "TestFS.h"
+#include "Threading.h"
 #include "URI.h"
 #include "clang/Config/config.h"
+#include "clang/Sema/CodeCompleteConsumer.h"
 #include "llvm/ADT/SmallVector.h"
 #include "llvm/ADT/StringMap.h"
 #include "llvm/Support/Errc.h"
@@ -36,7 +38,6 @@
 namespace {
 
 using ::testing::ElementsAre;
-using ::testing::Eq;
 using ::testing::Field;
 using ::testing::Gt;
 using ::testing::IsEmpty;
@@ -1058,6 +1059,55 @@
   EXPECT_NE(Result, "");
 }
 
+TEST_F(ClangdVFSTest, FallbackWhenPreambleIsNotReady) {
+  MockFSProvider FS;
+  ErrorCheckingDiagConsumer DiagConsumer;
+  // Returns compile command only when notified.
+  class DelayedCompilationDatabase : public GlobalCompilationDatabase {
+  public:
+DelayedCompilationDatabase(Notification )
+: CanReturnCommand(CanReturnCommand) {}
+
+llvm::Optional
+getCompileCommand(PathRef File, ProjectInfo * = nullptr) const override {
+  CanReturnCommand.wait();
+  auto FileName = llvm::sys::path::filename(File);

[PATCH] D60116: [clang-format] Regroup #includes into blocks for Google style

[Eric Liu via Phabricator via cfe-commits]

ioeric created this revision.
ioeric added reviewers: sammccall, klimek.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

Regrouping #includes in blocks separated by blank lines when sorting C++ 
#include
headers was implemented recently, and it has been preferred in Google's C++ 
style guide:
https://google.github.io/styleguide/cppguide.html#Names_and_Order_of_Includes


Repository:
  rC Clang

https://reviews.llvm.org/D60116

Files:
  lib/Format/Format.cpp
  unittests/Format/CleanupTest.cpp
  unittests/Format/SortIncludesTest.cpp


Index: unittests/Format/SortIncludesTest.cpp
===
--- unittests/Format/SortIncludesTest.cpp
+++ unittests/Format/SortIncludesTest.cpp
@@ -272,6 +272,7 @@
   FmtStyle = getGoogleStyle(FormatStyle::LK_Cpp);
   EXPECT_EQ("#include \n"
 "#include \n"
+"\n"
 "#include \"a.h\"\n"
 "#include \"c.h\"\n",
 sort("#include \n"
Index: unittests/Format/CleanupTest.cpp
===
--- unittests/Format/CleanupTest.cpp
+++ unittests/Format/CleanupTest.cpp
@@ -420,8 +420,10 @@
 TEST_F(CleanUpReplacementsTest, InsertMultipleNewHeadersAndSortGoogle) {
   std::string Code = "\nint x;";
   std::string Expected = "\n#include \"fix.h\"\n"
+ "\n"
  "#include \n"
  "#include \n"
+ "\n"
  "#include \"a.h\"\n"
  "#include \"b.h\"\n"
  "#include \"c.h\"\n"
Index: lib/Format/Format.cpp
===
--- lib/Format/Format.cpp
+++ lib/Format/Format.cpp
@@ -784,6 +784,7 @@
   GoogleStyle.IncludeStyle.IncludeCategories = {
   {"^", 2}, {"^<.*\\.h>", 1}, {"^<.*", 2}, {".*", 3}};
   GoogleStyle.IncludeStyle.IncludeIsMainRegex = "([-_](test|unittest))?$";
+  GoogleStyle.IncludeStyle.IncludeBlocks = tooling::IncludeStyle::IBS_Regroup;
   GoogleStyle.IndentCaseLabels = true;
   GoogleStyle.KeepEmptyLinesAtTheStartOfBlocks = false;
   GoogleStyle.ObjCBinPackProtocolList = FormatStyle::BPS_Never;


Index: unittests/Format/SortIncludesTest.cpp
===
--- unittests/Format/SortIncludesTest.cpp
+++ unittests/Format/SortIncludesTest.cpp
@@ -272,6 +272,7 @@
   FmtStyle = getGoogleStyle(FormatStyle::LK_Cpp);
   EXPECT_EQ("#include \n"
 "#include \n"
+"\n"
 "#include \"a.h\"\n"
 "#include \"c.h\"\n",
 sort("#include \n"
Index: unittests/Format/CleanupTest.cpp
===
--- unittests/Format/CleanupTest.cpp
+++ unittests/Format/CleanupTest.cpp
@@ -420,8 +420,10 @@
 TEST_F(CleanUpReplacementsTest, InsertMultipleNewHeadersAndSortGoogle) {
   std::string Code = "\nint x;";
   std::string Expected = "\n#include \"fix.h\"\n"
+ "\n"
  "#include \n"
  "#include \n"
+ "\n"
  "#include \"a.h\"\n"
  "#include \"b.h\"\n"
  "#include \"c.h\"\n"
Index: lib/Format/Format.cpp
===
--- lib/Format/Format.cpp
+++ lib/Format/Format.cpp
@@ -784,6 +784,7 @@
   GoogleStyle.IncludeStyle.IncludeCategories = {
   {"^", 2}, {"^<.*\\.h>", 1}, {"^<.*", 2}, {".*", 3}};
   GoogleStyle.IncludeStyle.IncludeIsMainRegex = "([-_](test|unittest))?$";
+  GoogleStyle.IncludeStyle.IncludeBlocks = tooling::IncludeStyle::IBS_Regroup;
   GoogleStyle.IndentCaseLabels = true;
   GoogleStyle.KeepEmptyLinesAtTheStartOfBlocks = false;
   GoogleStyle.ObjCBinPackProtocolList = FormatStyle::BPS_Never;
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D59811: [clangd] Add fallback mode for code completion when compile command or preamble is not ready.

[Eric Liu via Phabricator via cfe-commits]

ioeric added a comment.

In D59811#1445701 , @ilya-biryukov 
wrote:

> I think this option should be configurable (and off by default) for the 
> transition period. A few reasons to do so:
>
> - Before we have an actual implementation of fallback completions, the 
> current behavior (waiting for the first preamble) actually seems like a 
> better experience than returning empty results.
> - Some clients do this kind of fallback on their own (e.g. VSCode), so until 
> we can provide actually semantically interesting results (anything more than 
> identifier-based really, e.g. keyword completions) we would be better off 
> keeping it off.
> - We can still test it if it's off by flipping the corresponding flag in the 
> test code.
> - Would make rollout easier (clients can flip the flag back and forth and 
> make sure it does not break stuff for them)


All very good points. Thanks! I've added an option.




Comment at: clangd/ClangdServer.cpp:197
   return CB(llvm::make_error());
+if (!IP->Preamble) {
+  vlog("File {0} is not ready for code completion. Enter fallback mode.",

ilya-biryukov wrote:
> This way we don't distinguish between the failure to build a preamble and the 
> fallback mode.
> Have you considered introducing a different callback instead to clearly 
> separate two cases for the clients?
> The code handling those would hardly have any similarities anyway, given that 
> the nature of those two cases is so different.
> 
> Would look something like this:
> ```
> /// When \p FallbackAction is not null, it would be called instead of \p 
> Action in cases when preamble is 
> /// not yet ready.
> void runWithPreamble(… Callback Action, Callback 
> FallbackAction);
> ```
> void runWithPreamble(… Callback Action, Callback 
> FallbackAction);
This is what I started with in the first iteration. The main problem is that we 
would need to bind `CB` to both actions; however, `CB` is not 
copyable/shareable by design, I think. This seems to leave us with the only 
option to handle everything in the same action. I thought this was fine because 
an parameter `AllowFallback` seems to be clear as well. I'm open to suggestions 
;)

> This way we don't distinguish between the failure to build a preamble and the 
> fallback mode.
I am assuming that a fallback for `runWithPreamble` is either compile command 
is missing/broken or preamble fails to be built. And in both cases, fallback 
mode could be useful. Do you think we should have more distinctions made here?



Comment at: clangd/TUScheduler.cpp:186
 
   std::shared_ptr getPossiblyStalePreamble() const;
+

ilya-biryukov wrote:
> Could we put the preamble and compile command into a single function?
> Getting them in the lock-step would mean they're always consistent, which is 
> a good thing.
Could you elaborate the advantages of keeping them consistent?

They seem to be updated and used relatively independently. Grouping them into 
one function seems to add more work to the call sites.  And it seems inevitable 
that we would some inconsistency between the two.



Comment at: clangd/TUScheduler.cpp:371
+  std::lock_guard Lock(Mutex);
+  this->CompileCommand = Inputs.CompileCommand;
+}

ilya-biryukov wrote:
> After this point the clients might start getting a new compile command and an 
> old preamble.
> This seems fine (the clients should be ready for this), but let's document it 
> in the methods that expose a compile command and a preamble.
> After this point the clients might start getting a new compile command and an 
> old preamble.
Did we have guarantee about this before? It seems that we could also have the 
similar situation.

> This seems fine (the clients should be ready for this), but let's document it 
> in the methods that expose a compile command and a preamble.
I added documentation for `getCurrentPreamble`; would it be the right place? 
`getPossiblyStalePreamble` seems self-explaining enough. And since compile 
command is always fresher than preamble, I think it would be fine without 
commenting?



Comment at: clangd/TUScheduler.cpp:918
+  // asynchronous mode, as TU update should finish before this is run.
+  if (!It->second->Worker->isFirstPreambleBuilt() && AllowFallback &&
+  PreambleTasks) {

ilya-biryukov wrote:
> It would be better to make a decision on whether to use a fallback mode in 
> the actual function scheduled on a different thread (i.e. inside the body of 
> `Task`).
> Imagine the preamble is being built right now and would finish before 
> scheduling this task. In that case we would have a chance to hit the 
> "preamble" if we make a decision in the body of the handler, but won't have 
> that chance in the current implementation.
It sounds like a very small window (~ms?) that we are trying to optimize for.  
Given that users type 

[PATCH] D59811: [clangd] Add fallback mode for code completion when compile command or preamble is not ready.

[Eric Liu via Phabricator via cfe-commits]

ioeric updated this revision to Diff 192683.
ioeric marked 9 inline comments as done.
ioeric added a comment.

- address review comments.


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D59811

Files:
  clangd/ClangdServer.cpp
  clangd/CodeComplete.h
  clangd/TUScheduler.cpp
  clangd/TUScheduler.h
  clangd/Threading.h
  clangd/tool/ClangdMain.cpp
  unittests/clangd/ClangdTests.cpp
  unittests/clangd/TUSchedulerTests.cpp

Index: unittests/clangd/TUSchedulerTests.cpp
===
--- unittests/clangd/TUSchedulerTests.cpp
+++ unittests/clangd/TUSchedulerTests.cpp
@@ -15,14 +15,13 @@
 #include "gmock/gmock.h"
 #include "gtest/gtest.h"
 #include 
+#include 
 #include 
 
 namespace clang {
 namespace clangd {
 namespace {
 
-using ::testing::_;
-using ::testing::AllOf;
 using ::testing::AnyOf;
 using ::testing::Each;
 using ::testing::ElementsAre;
@@ -35,13 +34,18 @@
 
 class TUSchedulerTests : public ::testing::Test {
 protected:
-  ParseInputs getInputs(PathRef File, std::string Contents) {
+  FileUpdateInputs getInputs(PathRef File, std::string Contents) {
 ParseInputs Inputs;
-Inputs.CompileCommand = *CDB.getCompileCommand(File);
 Inputs.FS = buildTestFS(Files, Timestamps);
 Inputs.Contents = std::move(Contents);
 Inputs.Opts = ParseOptions();
-return Inputs;
+FileUpdateInputs UpdateInputs;
+UpdateInputs.InputSansCommand = std::move(Inputs);
+std::string FilePath = File;
+UpdateInputs.GetCompileCommand = [this, FilePath]() {
+  return *CDB.getCompileCommand(FilePath);
+};
+return UpdateInputs;
   }
 
   void updateWithCallback(TUScheduler , PathRef File,
@@ -72,7 +76,7 @@
   /// Schedule an update and call \p CB with the diagnostics it produces, if
   /// any. The TUScheduler should be created with captureDiags as a
   /// DiagsCallback for this to work.
-  void updateWithDiags(TUScheduler , PathRef File, ParseInputs Inputs,
+  void updateWithDiags(TUScheduler , PathRef File, FileUpdateInputs Inputs,
WantDiagnostics WD,
llvm::unique_function)> CB) {
 Path OrigFile = File.str();
@@ -245,7 +249,7 @@
 
 S.runWithPreamble("StaleRead", Path, TUScheduler::Stale,
   [&](Expected Pre) {
-ASSERT_TRUE(bool(Pre));
+EXPECT_TRUE((bool)Pre);
 assert(bool(Pre));
 EXPECT_THAT(includes(Pre->Preamble),
 ElementsAre(""));
@@ -397,8 +401,9 @@
 EXPECT_THAT(Context::current().get(NonceKey), Pointee(Nonce));
 
 ASSERT_TRUE((bool)AST);
-EXPECT_EQ(AST->Inputs.FS, Inputs.FS);
-EXPECT_EQ(AST->Inputs.Contents, Inputs.Contents);
+EXPECT_EQ(AST->Inputs.FS, Inputs.InputSansCommand.FS);
+EXPECT_EQ(AST->Inputs.Contents,
+  Inputs.InputSansCommand.Contents);
 
 std::lock_guard Lock(Mut);
 ++TotalASTReads;
@@ -415,7 +420,7 @@
 EXPECT_THAT(Context::current().get(NonceKey), Pointee(Nonce));
 
 ASSERT_TRUE((bool)Preamble);
-EXPECT_EQ(Preamble->Contents, Inputs.Contents);
+EXPECT_EQ(Preamble->Contents, Inputs.InputSansCommand.Contents);
 
 std::lock_guard Lock(Mut);
 ++TotalPreambleReads;
@@ -504,6 +509,7 @@
   )cpp";
   auto WithEmptyPreamble = R"cpp(int main() {})cpp";
   S.update(Foo, getInputs(Foo, WithPreamble), WantDiagnostics::Auto);
+
   S.runWithPreamble(
   "getNonEmptyPreamble", Foo, TUScheduler::Stale,
   [&](Expected Preamble) {
Index: unittests/clangd/ClangdTests.cpp
===
--- unittests/clangd/ClangdTests.cpp
+++ unittests/clangd/ClangdTests.cpp
@@ -13,8 +13,10 @@
 #include "Matchers.h"
 #include "SyncAPI.h"
 #include "TestFS.h"
+#include "Threading.h"
 #include "URI.h"
 #include "clang/Config/config.h"
+#include "clang/Sema/CodeCompleteConsumer.h"
 #include "llvm/ADT/SmallVector.h"
 #include "llvm/ADT/StringMap.h"
 #include "llvm/Support/Errc.h"
@@ -36,7 +38,6 @@
 namespace {
 
 using ::testing::ElementsAre;
-using ::testing::Eq;
 using ::testing::Field;
 using ::testing::Gt;
 using ::testing::IsEmpty;
@@ -1058,6 +1059,55 @@
   EXPECT_NE(Result, "");
 }
 
+TEST_F(ClangdVFSTest, FallbackWhenPreambleIsNotReady) {
+  MockFSProvider FS;
+  ErrorCheckingDiagConsumer DiagConsumer;
+  // Returns compile command only when notified.
+  class DelayedCompilationDatabase : public GlobalCompilationDatabase {
+  public:
+DelayedCompilationDatabase(Notification )
+: CanReturnCommand(CanReturnCommand) {}
+
+llvm::Optional
+getCompileCommand(PathRef File, ProjectInfo * = nullptr) const override {
+  

[PATCH] D59811: [clangd] Add fallback mode for code completion when compile command or preamble is not ready.

[Eric Liu via Phabricator via cfe-commits]

ioeric created this revision.
ioeric added a reviewer: ilya-biryukov.
Herald added subscribers: cfe-commits, jdoerfert, kadircet, arphaman, jkorous, 
MaskRay, javed.absar.
Herald added a project: clang.

When calling TUScehduler::runWithPreamble (e.g. in code compleiton), allow
entering a fallback mode when compile command or preamble is not ready, instead 
of
waiting. This allows clangd to perform naive code completion e.g. using 
identifiers
in the current file or symbols in the index.

This patch simply returns empty result for code completion in fallback mode. 
Identifier-based
plus more advanced index-based completion will be added in followup patches.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D59811

Files:
  clangd/ClangdServer.cpp
  clangd/TUScheduler.cpp
  clangd/TUScheduler.h
  clangd/Threading.h
  unittests/clangd/ClangdTests.cpp
  unittests/clangd/CodeCompleteTests.cpp
  unittests/clangd/TUSchedulerTests.cpp

Index: unittests/clangd/TUSchedulerTests.cpp
===
--- unittests/clangd/TUSchedulerTests.cpp
+++ unittests/clangd/TUSchedulerTests.cpp
@@ -15,14 +15,13 @@
 #include "gmock/gmock.h"
 #include "gtest/gtest.h"
 #include 
+#include 
 #include 
 
 namespace clang {
 namespace clangd {
 namespace {
 
-using ::testing::_;
-using ::testing::AllOf;
 using ::testing::AnyOf;
 using ::testing::Each;
 using ::testing::ElementsAre;
@@ -35,13 +34,18 @@
 
 class TUSchedulerTests : public ::testing::Test {
 protected:
-  ParseInputs getInputs(PathRef File, std::string Contents) {
+  FileUpdateInputs getInputs(PathRef File, std::string Contents) {
 ParseInputs Inputs;
-Inputs.CompileCommand = *CDB.getCompileCommand(File);
 Inputs.FS = buildTestFS(Files, Timestamps);
 Inputs.Contents = std::move(Contents);
 Inputs.Opts = ParseOptions();
-return Inputs;
+FileUpdateInputs UpdateInputs;
+UpdateInputs.InputSansCommand = std::move(Inputs);
+std::string FilePath = File;
+UpdateInputs.GetCompileCommand = [this, FilePath]() {
+  return *CDB.getCompileCommand(FilePath);
+};
+return UpdateInputs;
   }
 
   void updateWithCallback(TUScheduler , PathRef File,
@@ -72,7 +76,7 @@
   /// Schedule an update and call \p CB with the diagnostics it produces, if
   /// any. The TUScheduler should be created with captureDiags as a
   /// DiagsCallback for this to work.
-  void updateWithDiags(TUScheduler , PathRef File, ParseInputs Inputs,
+  void updateWithDiags(TUScheduler , PathRef File, FileUpdateInputs Inputs,
WantDiagnostics WD,
llvm::unique_function)> CB) {
 Path OrigFile = File.str();
@@ -245,7 +249,7 @@
 
 S.runWithPreamble("StaleRead", Path, TUScheduler::Stale,
   [&](Expected Pre) {
-ASSERT_TRUE(bool(Pre));
+EXPECT_TRUE((bool)Pre);
 assert(bool(Pre));
 EXPECT_THAT(includes(Pre->Preamble),
 ElementsAre(""));
@@ -397,8 +401,9 @@
 EXPECT_THAT(Context::current().get(NonceKey), Pointee(Nonce));
 
 ASSERT_TRUE((bool)AST);
-EXPECT_EQ(AST->Inputs.FS, Inputs.FS);
-EXPECT_EQ(AST->Inputs.Contents, Inputs.Contents);
+EXPECT_EQ(AST->Inputs.FS, Inputs.InputSansCommand.FS);
+EXPECT_EQ(AST->Inputs.Contents,
+  Inputs.InputSansCommand.Contents);
 
 std::lock_guard Lock(Mut);
 ++TotalASTReads;
@@ -415,7 +420,7 @@
 EXPECT_THAT(Context::current().get(NonceKey), Pointee(Nonce));
 
 ASSERT_TRUE((bool)Preamble);
-EXPECT_EQ(Preamble->Contents, Inputs.Contents);
+EXPECT_EQ(Preamble->Contents, Inputs.InputSansCommand.Contents);
 
 std::lock_guard Lock(Mut);
 ++TotalPreambleReads;
@@ -504,6 +509,7 @@
   )cpp";
   auto WithEmptyPreamble = R"cpp(int main() {})cpp";
   S.update(Foo, getInputs(Foo, WithPreamble), WantDiagnostics::Auto);
+
   S.runWithPreamble(
   "getNonEmptyPreamble", Foo, TUScheduler::Stale,
   [&](Expected Preamble) {
Index: unittests/clangd/CodeCompleteTests.cpp
===
--- unittests/clangd/CodeCompleteTests.cpp
+++ unittests/clangd/CodeCompleteTests.cpp
@@ -1358,6 +1358,7 @@
   // parsing.
   CDB.ExtraClangFlags.push_back("-fno-delayed-template-parsing");
   Server.addDocument(FooCpp, Source.code(), WantDiagnostics::Yes);
+  ASSERT_TRUE(Server.blockUntilIdleForTest());
   CodeCompleteResult Completions = cantFail(runCodeComplete(
   Server, FooCpp, Source.point(), clangd::CodeCompleteOptions()));
 
Index: unittests/clangd/ClangdTests.cpp
===
--- unittests/clangd/ClangdTests.cpp
+++ 

[PATCH] D59700: Make clang-move use same file naming convention as other tools

[Eric Liu via Phabricator via cfe-commits]

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

lgtm


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

https://reviews.llvm.org/D59700



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

[PATCH] D59639: [clangd] Print template arguments helper

[Eric Liu via Phabricator via cfe-commits]

ioeric added inline comments.



Comment at: clang-tools-extra/clangd/AST.cpp:149
+} else {
+  // FIXME: Fix cases when getTypeAsWritten returns null, e.g. friend 
decls.
+  printTemplateArgumentList(OS, Cls->getTemplateArgs().asArray(), Policy);

I'm not quite sure if I understand this FIXME. In this `else` branch, we are 
handling this case. Do you mean this is not a proper fix and future work is 
needed here? Could you elaborate?

One thing that's worth commenting is why we could use 
`Cls->getTemplateArgs().asArray()` when `Cls->getTypeAsWritten` is null. It's 
not trivial from the code.



Comment at: clang-tools-extra/clangd/AST.cpp:88
 static const TemplateArgumentList *
 getTemplateSpecializationArgs(const NamedDecl ) {
   if (auto *Func = llvm::dyn_cast())

kadircet wrote:
> ioeric wrote:
> > can we unify this with `getTemplateSpecializationArgLocs` somehow? 
> > 
> > it seems that args as written would also be favorable here (see FIXME in 
> > line 112)
> See D59641
thought?



Comment at: clang-tools-extra/clangd/AST.cpp:133
+printTemplateArgumentList(OS, *Args, Policy);
+  else if (auto *Cls = llvm::dyn_cast()) {
+if (const TypeSourceInfo *TSI = Cls->getTypeAsWritten()) {

kadircet wrote:
> ioeric wrote:
> > why isn't this handled in `getTemplateSpecializationArgLocs`? Add a comment?
> it is mentioned in `getTemplateSpecializationArgLocs`, would you rather move 
> the comment here?
I think it would be clearer if we do something like:

```
if (auto *Cls = llvm::dyn_cast()) {
  // handle this specially because ...
} else {
  // use getTemplateSpecializationArgLocs to handle rest of cases.
}

```



Comment at: clang/lib/AST/TypePrinter.cpp:1635
 
+static void printArgument(const TemplateArgument , const PrintingPolicy ,
+  llvm::raw_ostream ) {

kadircet wrote:
> ioeric wrote:
> > could you add clang tests for these changes?
> We've also discussed this in the original comment and decided infrastructure 
> necessary was too overwhelming. First we need to invoke compiler and get the 
> AST, then write a Visitor to fetch relevant decls and only after that call 
> printTemplateArguments ...
No test at all is concerning... I think there are lit tests for AST printing in 
test/AST/. Is it possible to have some of those?



Comment at: clang/lib/AST/TypePrinter.cpp:1640
+
+static void printArgument(const TemplateArgumentLoc ,
+  const PrintingPolicy , llvm::raw_ostream ) {

kadircet wrote:
> ioeric wrote:
> > It's unclear to me what the new behavior is with changes in this file. 
> > Could you add comment?
> > 
> > It might make sense to split the clang change into a separate patch and get 
> > folks who are more familiar to take a look.
> it just prevents erasure from TypeLoc to Type when printing the argument, so 
> that we can do a fine-grained printing if Location is already there.
could you add this into comment?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D59639



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

[PATCH] D59683: [Tooling] Avoid working-dir races in AllTUsToolExecutor

[Eric Liu via Phabricator via cfe-commits]

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

Neat!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D59683



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

[PATCH] D59639: [clangd] Print template arguments helper

[Eric Liu via Phabricator via cfe-commits]

ioeric added inline comments.



Comment at: clang/lib/AST/TypePrinter.cpp:1640
+
+static void printArgument(const TemplateArgumentLoc ,
+  const PrintingPolicy , llvm::raw_ostream ) {

It's unclear to me what the new behavior is with changes in this file. Could 
you add comment?

It might make sense to split the clang change into a separate patch and get 
folks who are more familiar to take a look.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D59639



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

[PATCH] D59640: [clangd] Add TemplateArgumentList into Symbol

[Eric Liu via Phabricator via cfe-commits]

ioeric added a comment.

should we update YAML?




Comment at: clang-tools-extra/clangd/index/Symbol.h:48
+  /// non-specializations. Example: ""
+  llvm::StringRef TemplateArgumentList;
   /// The location of the symbol's definition, if one was found.

How about  `TemplateSpecializationArgs`? 

Could you also put this field near `ReturnType` or `Signature`? 



Comment at: clang-tools-extra/clangd/index/SymbolCollector.cpp:526
   S.ID = std::move(ID);
+  std::string TemplateArgumentList = printTemplateArgsAsWritten(ND);
+  S.TemplateArgumentList = TemplateArgumentList;

put this near `ReturnType` initialization.



Comment at: clang-tools-extra/unittests/clangd/SymbolCollectorTests.cpp:396
 
 TEST_F(SymbolCollectorTest, Template) {
   Annotations Header(R"(

this test is getting hard to read. could you only make minimum change to the 
existing test and add a new case for the new behavior?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D59640



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

[PATCH] D59639: [clangd] Print template arguments helper

[Eric Liu via Phabricator via cfe-commits]

ioeric added inline comments.



Comment at: clang-tools-extra/clangd/AST.cpp:88
 static const TemplateArgumentList *
 getTemplateSpecializationArgs(const NamedDecl ) {
   if (auto *Func = llvm::dyn_cast())

can we unify this with `getTemplateSpecializationArgLocs` somehow? 

it seems that args as written would also be favorable here (see FIXME in line 
112)



Comment at: clang-tools-extra/clangd/AST.cpp:131
+  PrintingPolicy Policy(ND.getASTContext().getLangOpts());
+  if (auto Args = getTemplateSpecializationArgLocs(ND))
+printTemplateArgumentList(OS, *Args, Policy);

Eugene.Zelenko wrote:
> Return type is not obvious, so auto should not be used.
nit: I'd suggest keeping `{}` for symmetry.



Comment at: clang-tools-extra/clangd/AST.cpp:133
+printTemplateArgumentList(OS, *Args, Policy);
+  else if (auto *Cls = llvm::dyn_cast()) {
+if (const TypeSourceInfo *TSI = Cls->getTypeAsWritten()) {

why isn't this handled in `getTemplateSpecializationArgLocs`? Add a comment?



Comment at: clang-tools-extra/clangd/AST.cpp:134
+  else if (auto *Cls = llvm::dyn_cast()) {
+if (const TypeSourceInfo *TSI = Cls->getTypeAsWritten()) {
+  auto STL = TSI->getTypeLoc().getAs();

could you add comment/example explaining what's special about this case?



Comment at: clang-tools-extra/clangd/AST.h:54
+/// Returns an empty string if type is not a template specialization.
+std::string printTemplateArgsAsWritten(const NamedDecl );
+

Maybe `printTemplateSpecializationArgs` since we are only handling template 
specialization? 

I think we could drop `AsWritten` from the name. It should be clear to just 
explain in the comment.  



Comment at: clang-tools-extra/unittests/clangd/ASTUtilsTests.cpp:16
+
+TEST(ASTUtils, PrintTemplateArgs) {
+  Annotations Test(R"cpp(

I think this kind of tests would be more readable using `TEST_P` [1] with code 
and expected arguments as parameters.

Up to you though.

[1] 
https://github.com/google/googletest/blob/master/googletest/docs/advanced.md#specifying-names-for-value-parameterized-test-parameters



Comment at: clang/lib/AST/TypePrinter.cpp:1635
 
+static void printArgument(const TemplateArgument , const PrintingPolicy ,
+  llvm::raw_ostream ) {

could you add clang tests for these changes?



Comment at: clang/lib/AST/TypePrinter.cpp:1643
+  const auto  = A.getArgument().getKind();
+  assert(Kind != TemplateArgument::Null &&
+ "TemplateArgumentKind can not be null!");

why? 

```
/// Represents an empty template argument, e.g., one that has not
/// been deduced.
```
It seems legitimate. 

Since this hot code path, we would want to avoid hard assertion if possible.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D59639



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

[PATCH] D59481: [clangd] Count number of references while merging RefSlabs inside FileIndex

[Eric Liu via Phabricator via cfe-commits]

ioeric added inline comments.



Comment at: clang-tools-extra/clangd/index/FileIndex.cpp:33
+// If MergedSyms is provided, increments a symbols Reference count once for
+// every file it was mentioned in.
+void mergeRefs(llvm::ArrayRef> RefSlabs,

nit: s/every file/every slab/?

There is no "file" in this context.



Comment at: clang-tools-extra/clangd/index/FileIndex.cpp:46
+auto It = MergedSyms->find(Sym.first);
+assert(It != MergedSyms->end() && "Reference to unknown symbol!");
+// Note that we increment References per each file mentioning the

I think we can trigger this assertion with an incomplete background index. For 
example, we've seen references of a symbol in some files but we've not parsed 
the file that contains the decls.



Comment at: clang-tools-extra/clangd/index/FileIndex.cpp:50
+// FileIndex keeps only oneslab per file.
+// This contradicts with behavior inside clangd-indexer, it only counts
+// translation units since it processes each header multiple times.

kadircet wrote:
> ioeric wrote:
> > this is a bit of a code smell. FileIndex is not supposed to know anything 
> > about clangd-indexer etc.
> I've actually just left the comment for review so that we can decide what to 
> do about discrepancy(with my reasoning). It is not like FileIndex is somehow 
> tied to clangd-indexer now?
the comment is useful. I just think it's in the wrong place. If we define the 
reference counting behavior for `FileSymbols`, `FileSymbols`wouldn't need to 
know about clangd-indexer or background-index. This comment can then live in 
background index instead.



Comment at: clang-tools-extra/clangd/index/FileIndex.cpp:170
   I.first->second = mergeSymbol(I.first->second, Sym);
+// We re-count number of references while merging refs from scratch.
+I.first->getSecond().References = 0;

kadircet wrote:
> ioeric wrote:
> > kadircet wrote:
> > > ioeric wrote:
> > > > note that references might not always exist. SymbolCollector can be set 
> > > > up to not collect references.
> > > I thought we wouldn't make any promises about `References` in such a 
> > > case, do we?
> > Is this documented somewhere?
> > 
> > `References` and `RefSlab` have been two independent things. `References` 
> > existed before `RefSlab`. Now might be a good time to unify them, but I 
> > think we should think about how this is reflected in their definitions 
> > (documentation etc) and other producers and consumers of the structures.
> Both of them are only produced by SymbolCollector, which has an option 
> `CountReferences` with a comment saying `// Populate the Symbol.References 
> field.`.
> Unfortunately that's not what it does, instead it always sets 
> `Symol.References` field to `1` for every symbol collected during indexing of 
> a single TU.
> Then it is up to the consumers to merge those `1`s. So even though we say:
> ```
> /// The number of translation units that reference this symbol from their main
> /// file. This number is only meaningful if aggregated in an index.```
> in the comments of `Symbol::References` it is totally up-to-the consumer who 
> performs the merging.
> 
> The option controls whether Symbol Occurences should be collected or not.
> 
> I see two possible options:
>  - Un-tie SymbolCollector from Symbol::References and rather define it as:
> ```
> /// This field is for the convenience of an aggregated symbol index
> ```
>  - Rather change Index structure's(consumers of this information) to 
> store(and build) this information internally if they plan to make use of it ?
> 
> WDYT?
> Unfortunately that's not what it does, instead it always sets 
> Symol.References field to 1 for every symbol collected during indexing of a 
> single TU.
Looking at the code, I think `Symol.References` is indeed only populated (i.e. 
set to 1) when `CountReferences` is enabled. Honestly, we should probably get 
rid of `CountReferences`; I don't see a reason not to count references. 

But the statement `count one per TU` still stands in SymbolCollector, as it 
always only handles one TU at a time. 

Actually, all I'm asking in the initial comment is to define the `References` 
counting behavior when there is no reference ;) I think a reasonable solution 
is to keep the `references` untouched when there is no reference, so that 
`References` aggregation via `mergeSymbol` can still work (for one-TU-per-Slab 
use cases). 

And we should probably also document the new behavior e.g. `DuplicateHandling` 
in FileIndex.h seems to be a good place.



Comment at: clang-tools-extra/clangd/index/FileIndex.cpp:180
 }
 // FIXME: aggregate symbol reference count based on references.
 break;

this FIXME can be removed?



Comment at: clang-tools-extra/clangd/index/FileIndex.cpp:189

[PATCH] D59487: [Tooling] Add more scope specifiers until spelling is not ambiguous.

[Eric Liu via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
ioeric marked an inline comment as done.
Closed by commit rL356446: [Tooling] Add more scope specifiers until spelling 
is not ambiguous. (authored by ioeric, committed by ).
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.

Repository:
  rL LLVM

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

https://reviews.llvm.org/D59487

Files:
  cfe/trunk/lib/Tooling/Core/Lookup.cpp
  cfe/trunk/unittests/Tooling/LookupTest.cpp

Index: cfe/trunk/lib/Tooling/Core/Lookup.cpp
===
--- cfe/trunk/lib/Tooling/Core/Lookup.cpp
+++ cfe/trunk/lib/Tooling/Core/Lookup.cpp
@@ -14,6 +14,7 @@
 #include "clang/AST/Decl.h"
 #include "clang/AST/DeclCXX.h"
 #include "clang/AST/DeclarationName.h"
+#include "llvm/ADT/SmallVector.h"
 using namespace clang;
 using namespace clang::tooling;
 
@@ -114,35 +115,60 @@
   return false;
 }
 
-// Returns true if spelling symbol \p QName as \p Spelling in \p UseContext is
-// ambiguous. For example, if QName is "::y::bar" and the spelling is "y::bar"
-// in `UseContext` "a" that contains a nested namespace "a::y", then "y::bar"
-// can be resolved to ::a::y::bar, which can cause compile error.
+// Adds more scope specifier to the spelled name until the spelling is not
+// ambiguous. A spelling is ambiguous if the resolution of the symbol is
+// ambiguous. For example, if QName is "::y::bar", the spelling is "y::bar", and
+// context contains a nested namespace "a::y", then "y::bar" can be resolved to
+// ::a::y::bar in the context, which can cause compile error.
 // FIXME: consider using namespaces.
-static bool isAmbiguousNameInScope(StringRef Spelling, StringRef QName,
-   const DeclContext ) {
+static std::string disambiguateSpellingInScope(StringRef Spelling,
+   StringRef QName,
+   const DeclContext ) {
   assert(QName.startswith("::"));
+  assert(QName.endswith(Spelling));
   if (Spelling.startswith("::"))
-return false;
+return Spelling;
 
-  // Lookup the first component of Spelling in all enclosing namespaces and
-  // check if there is any existing symbols with the same name but in different
-  // scope.
-  StringRef Head = Spelling.split("::").first;
+  auto UnspelledSpecifier = QName.drop_back(Spelling.size());
+  llvm::SmallVector UnspelledScopes;
+  UnspelledSpecifier.split(UnspelledScopes, "::", /*MaxSplit=*/-1,
+   /*KeepEmpty=*/false);
 
-  llvm::SmallVector UseNamespaces =
+  llvm::SmallVector EnclosingNamespaces =
   getAllNamedNamespaces();
   auto  = UseContext.getParentASTContext();
   StringRef TrimmedQName = QName.substr(2);
-  for (const auto *NS : UseNamespaces) {
-auto LookupRes = NS->lookup(DeclarationName((Head)));
-if (!LookupRes.empty()) {
-  for (const NamedDecl *Res : LookupRes)
-if (!TrimmedQName.startswith(Res->getQualifiedNameAsString()))
-  return true;
+
+  auto IsAmbiguousSpelling = [, , ](
+ const llvm::StringRef CurSpelling) {
+if (CurSpelling.startswith("::"))
+  return false;
+// Lookup the first component of Spelling in all enclosing namespaces
+// and check if there is any existing symbols with the same name but in
+// different scope.
+StringRef Head = CurSpelling.split("::").first;
+for (const auto *NS : EnclosingNamespaces) {
+  auto LookupRes = NS->lookup(DeclarationName((Head)));
+  if (!LookupRes.empty()) {
+for (const NamedDecl *Res : LookupRes)
+  if (!TrimmedQName.startswith(Res->getQualifiedNameAsString()))
+return true;
+  }
+}
+return false;
+  };
+
+  // Add more qualifiers until the spelling is not ambiguous.
+  std::string Disambiguated = Spelling;
+  while (IsAmbiguousSpelling(Disambiguated)) {
+if (UnspelledScopes.empty()) {
+  Disambiguated = "::" + Disambiguated;
+} else {
+  Disambiguated = (UnspelledScopes.back() + "::" + Disambiguated).str();
+  UnspelledScopes.pop_back();
 }
   }
-  return false;
+  return Disambiguated;
 }
 
 std::string tooling::replaceNestedName(const NestedNameSpecifier *Use,
@@ -179,12 +205,6 @@
   // specific).
   StringRef Suggested = getBestNamespaceSubstr(UseContext, ReplacementString,
isFullyQualified(Use));
-  // Use the fully qualified name if the suggested name is ambiguous.
-  // FIXME: consider re-shortening the name until the name is not ambiguous. We
-  // are not doing this because ambiguity is pretty bad and we should not try to
-  // be clever in handling such cases. Making this noticeable to users seems to
-  // be a better option.
-  return isAmbiguousNameInScope(Suggested, ReplacementString, *UseContext)
- ? ReplacementString
- : 

[PATCH] D59481: [clangd] Count number of references while merging RefSlabs inside FileIndex

[Eric Liu via Phabricator via cfe-commits]

ioeric added a comment.

> I don't follow why this can over-count, FileIndex keeps only one RefSlab per 
> each file, so we can't over-count? Did you mean it will be more than #TUs ?

Yes. This is how `Symbol::References` is described in its documentation. If we 
want to change/expand the semantic, we need to make it clear in the 
documentation as well.




Comment at: clang-tools-extra/clangd/index/FileIndex.cpp:170
   I.first->second = mergeSymbol(I.first->second, Sym);
+// We re-count number of references while merging refs from scratch.
+I.first->getSecond().References = 0;

kadircet wrote:
> ioeric wrote:
> > note that references might not always exist. SymbolCollector can be set up 
> > to not collect references.
> I thought we wouldn't make any promises about `References` in such a case, do 
> we?
Is this documented somewhere?

`References` and `RefSlab` have been two independent things. `References` 
existed before `RefSlab`. Now might be a good time to unify them, but I think 
we should think about how this is reflected in their definitions (documentation 
etc) and other producers and consumers of the structures.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D59481



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

[PATCH] D59487: [Tooling] Add more scope specifiers until spelling is not ambiguous.

[Eric Liu via Phabricator via cfe-commits]

ioeric marked 2 inline comments as done.
ioeric added inline comments.



Comment at: lib/Tooling/Core/Lookup.cpp:165
+if (UnspelledScopes.empty()) {
+  Disambiguated = "::" + Disambiguated;
+} else {

kadircet wrote:
> maybe return or break afterwards?
I also struggled a bit here. But I decided to let `IsAmbiguousSpelling` handle 
it because it seemed a bit more natural. Otherwise, we would somewhat duplicate 
the logic that spelling with leading "::" is not ambiguous.


Repository:
  rC Clang

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

https://reviews.llvm.org/D59487



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

[PATCH] D59487: [Tooling] Add more scope specifiers until spelling is not ambiguous.

[Eric Liu via Phabricator via cfe-commits]

ioeric created this revision.
ioeric added reviewers: kadircet, gribozavr.
Herald added subscribers: cfe-commits, jdoerfert.
Herald added a project: clang.

Previously, when the renamed spelling is ambiguous, we simply use the
full-qualfied name (with leading "::"). This patch makes it try adding
additional specifiers one at a time until name is no longer ambiguous,
which allows us to find better disambuguated spelling.


Repository:
  rC Clang

https://reviews.llvm.org/D59487

Files:
  lib/Tooling/Core/Lookup.cpp
  unittests/Tooling/LookupTest.cpp

Index: unittests/Tooling/LookupTest.cpp
===
--- unittests/Tooling/LookupTest.cpp
+++ unittests/Tooling/LookupTest.cpp
@@ -129,20 +129,38 @@
 
   // If the shortest name is ambiguous, we need to add more qualifiers.
   Visitor.OnCall = [&](CallExpr *Expr) {
-EXPECT_EQ("::a::y::bar", replaceCallExpr(Expr, "::a::y::bar"));
+EXPECT_EQ("a::y::bar", replaceCallExpr(Expr, "::a::y::bar"));
   };
   Visitor.runOver(R"(
 namespace a {
-namespace b {
-namespace x { void foo() {} }
-namespace y { void foo() {} }
-}
+ namespace b {
+  namespace x { void foo() {} }
+  namespace y { void foo() {} }
+ }
 }
 
 namespace a {
-namespace b {
-void f() { x::foo(); }
+ namespace b {
+  void f() { x::foo(); }
+ }
+})");
+
+  Visitor.OnCall = [&](CallExpr *Expr) {
+// y::bar would be ambiguous due to "a::b::y".
+EXPECT_EQ("::y::bar", replaceCallExpr(Expr, "::y::bar"));
+  };
+  Visitor.runOver(R"(
+namespace a {
+ namespace b {
+  void foo() {}
+  namespace y { }
+ }
 }
+
+namespace a {
+ namespace b {
+  void f() { foo(); }
+ }
 })");
 
   Visitor.OnCall = [&](CallExpr *Expr) {
Index: lib/Tooling/Core/Lookup.cpp
===
--- lib/Tooling/Core/Lookup.cpp
+++ lib/Tooling/Core/Lookup.cpp
@@ -14,6 +14,7 @@
 #include "clang/AST/Decl.h"
 #include "clang/AST/DeclCXX.h"
 #include "clang/AST/DeclarationName.h"
+#include "llvm/ADT/SmallVector.h"
 using namespace clang;
 using namespace clang::tooling;
 
@@ -114,35 +115,60 @@
   return false;
 }
 
-// Returns true if spelling symbol \p QName as \p Spelling in \p UseContext is
-// ambiguous. For example, if QName is "::y::bar" and the spelling is "y::bar"
-// in `UseContext` "a" that contains a nested namespace "a::y", then "y::bar"
-// can be resolved to ::a::y::bar, which can cause compile error.
+// Adds more scope specifier to the spelled name until the spelling is not
+// ambiguous. A spelling is ambiguous if the resolution of the symbol is
+// ambiguous. For example, if QName is "::y::bar", the spelling is "y::bar", and
+// context contains a nested namespace "a::y", then "y::bar" can be resolved to
+// ::a::y::bar in the context, which can cause compile error.
 // FIXME: consider using namespaces.
-static bool isAmbiguousNameInScope(StringRef Spelling, StringRef QName,
-   const DeclContext ) {
+static std::string disambiguateSpellingInScope(StringRef Spelling,
+   StringRef QName,
+   const DeclContext ) {
   assert(QName.startswith("::"));
+  assert(QName.endswith(Spelling));
   if (Spelling.startswith("::"))
-return false;
+return Spelling;
 
-  // Lookup the first component of Spelling in all enclosing namespaces and
-  // check if there is any existing symbols with the same name but in different
-  // scope.
-  StringRef Head = Spelling.split("::").first;
+  auto UnspelledSpecifier = QName.drop_back(Spelling.size());
+  llvm::SmallVector UnspelledScopes;
+  UnspelledSpecifier.split(UnspelledScopes, "::", /*MaxSplit=*/-1,
+   /*KeepEmpty=*/false);
 
-  llvm::SmallVector UseNamespaces =
+  llvm::SmallVector EnclosingNamespaces =
   getAllNamedNamespaces();
   auto  = UseContext.getParentASTContext();
   StringRef TrimmedQName = QName.substr(2);
-  for (const auto *NS : UseNamespaces) {
-auto LookupRes = NS->lookup(DeclarationName((Head)));
-if (!LookupRes.empty()) {
-  for (const NamedDecl *Res : LookupRes)
-if (!TrimmedQName.startswith(Res->getQualifiedNameAsString()))
-  return true;
+
+  auto IsAmbiguousSpelling = [, , ](
+ const llvm::StringRef CurSpelling) {
+if (CurSpelling.startswith("::"))
+  return false;
+// Lookup the first component of Spelling in all enclosing namespaces
+// and check if there is any existing symbols with the same name but in
+// different scope.
+StringRef Head = CurSpelling.split("::").first;
+for (const auto *NS : EnclosingNamespaces) {
+  auto LookupRes = NS->lookup(DeclarationName((Head)));
+  if (!LookupRes.empty()) {
+for (const NamedDecl *Res : LookupRes)
+  if 

[PATCH] D59481: [clangd] Count number of references while merging RefSlabs inside FileIndex

[Eric Liu via Phabricator via cfe-commits]

ioeric added a comment.

I'm not sure if FileIndex is the correct layer to make decision about how 
References is calculated. Currently, we have two use cases in clangd 1) one 
slab per TU and 2) one slab per file. It seems to me that we would want 
different strategies for the two use cases, so it might make sense to make this 
configurable in FileIndex.  And in one case, we have `References` ~= # of TUs, 
and in the other case, we would have `References` ~= # of files. This can 
over-count `References`  in the second case. It might be fine as an 
approximation (if there is no better solution), but we should definitely 
document it (e.g. in `BackgroundIndex`).




Comment at: clang-tools-extra/clangd/index/FileIndex.cpp:50
+// FileIndex keeps only oneslab per file.
+// This contradicts with behavior inside clangd-indexer, it only counts
+// translation units since it processes each header multiple times.

this is a bit of a code smell. FileIndex is not supposed to know anything about 
clangd-indexer etc.



Comment at: clang-tools-extra/clangd/index/FileIndex.cpp:170
   I.first->second = mergeSymbol(I.first->second, Sym);
+// We re-count number of references while merging refs from scratch.
+I.first->getSecond().References = 0;

note that references might not always exist. SymbolCollector can be set up to 
not collect references.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D59481



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

[PATCH] D59300: [clangd] Tune the fuzzy-matching algorithm

[Eric Liu via Phabricator via cfe-commits]

ioeric added inline comments.



Comment at: clang-tools-extra/clangd/FuzzyMatch.cpp:285
+  if (Pat[P] == Word[W] ||
+  (WordRole[W] == Head && (IsPatSingleCase || PatRole[P] == Head)))
 ++S;

ilya-biryukov wrote:
> ioeric wrote:
> > could you explain the intention of this change? Is it relevant to other 
> > changes in the patch? The original looks reasonable to me. 
> The motivation is keeping a bonus for matching the head of a segment even in 
> case of single-case patterns that lack segmentation signals.
> Previously, `p` from `up` would not get this bonus when matching 
> `unique_[p]tr` even though intuitively it should as we do want to encourage 
> those matches.
so, iiuc, when `IsPatSingleCase` is true, it means that any character in the 
pattern can potentially be a head? could you add comment for this?

We also had a stricter condition for `Pat[P] == Word[W]`, but now `ut` would 
get a bonus for `[u]nique_p[t]r`. Is this intended?



Comment at: clang-tools-extra/clangd/FuzzyMatch.cpp:273
   if (WordRole[W] == Head) // Skipping a segment.
-S += 1;
-  if (Last == Match) // Non-consecutive match.
-S += 2;  // We'd rather skip a segment than split our match.
-  return S;
+return 1; // We want to keep it lower than the bonus for a consecutive
+  // match.

nit: move this comment into into own line now that it's taking two?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D59300



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

[PATCH] D59300: [clangd] Tune the fuzzy-matching algorithm

[Eric Liu via Phabricator via cfe-commits]

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

(The result looks great)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D59300



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

[PATCH] D59300: [clangd] Tune the fuzzy-matching algorithm

[Eric Liu via Phabricator via cfe-commits]

ioeric added inline comments.



Comment at: clang-tools-extra/clangd/FuzzyMatch.cpp:74
 static bool isAwful(int S) { return S < AwfulScore / 2; }
-static constexpr int PerfectBonus = 3; // Perfect per-pattern-char score.
+static constexpr int PerfectBonus = 4; // Perfect per-pattern-char score.
 

could you explain this change?



Comment at: clang-tools-extra/clangd/FuzzyMatch.cpp:270
 int FuzzyMatcher::skipPenalty(int W, Action Last) const {
-  int S = 0;
+  if (W == 0) // Skipping the first character.
+return 3;

If the word is "_something", do we still want to penalize skipping first char?



Comment at: clang-tools-extra/clangd/FuzzyMatch.cpp:275
+  // match.
+  return 0;
 }

iiuc, there is no penalty by default because consecutive matches will gain 
bonus. This is not trivial here. Maybe add a comment? Or consider apply penalty 
for non-consecutive matches and no bonus for consecutive ones.



Comment at: clang-tools-extra/clangd/FuzzyMatch.cpp:285
+  if (Pat[P] == Word[W] ||
+  (WordRole[W] == Head && (IsPatSingleCase || PatRole[P] == Head)))
 ++S;

could you explain the intention of this change? Is it relevant to other changes 
in the patch? The original looks reasonable to me. 



Comment at: clang-tools-extra/unittests/clangd/FuzzyMatchTests.cpp:248
   EXPECT_THAT("foo", ranks("[foo]", "[Foo]"));
   EXPECT_THAT("onMes",
-  ranks("[onMes]sage", "[onmes]sage", "[on]This[M]ega[Es]capes"));

could you add a case for "onmes" pattern?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D59300



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

[PATCH] D58345: [clangd] Using symbol name to map includes for STL symbols.

[Eric Liu via Phabricator via cfe-commits]

ioeric accepted this revision.
ioeric added inline comments.
This revision is now accepted and ready to land.



Comment at: clangd/index/CanonicalIncludes.cpp:111
-  {"std::addressof", ""},
-  // Map symbols in  to their preferred includes.
   {"std::basic_filebuf", ""},

nit: as these symbols are not covered by the new mapping, I think this comment 
is still meaningful.


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D58345



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

[PATCH] D58345: [clangd] Using symbol name to map includes for STL symbols.

[Eric Liu via Phabricator via cfe-commits]

ioeric added inline comments.



Comment at: clangd/include-mapping/gen_std.py:46
+//
+// Generated from cppreference offline HTML book v%s.
+//===--===//

I'd suggest dropping `v` prefix as it might be confusing (e.g. one would search 
for the exact same version number on cppreference archive). Maybe add a short 
comment explaining that this is the modification date of the doc?



Comment at: clangd/include-mapping/gen_std.py:114
+  # so we use the modified time of the symbol_index.html as the version.
+  cppreference_version = datetime.datetime.fromtimestamp(
+os.stat(index_page_path).st_mtime).strftime('%Y-%m-%d')

s/version/date/, as this not really a version.



Comment at: clangd/include-mapping/gen_std.py:129
+headers = ParseSymbolPage(f.read())
+  if len(headers) == 0:
+sys.stderr.write("No header found for symbol %s at %s\n" % 
(symbol_name,

`if not headers:`



Comment at: clangd/index/CanonicalIncludes.cpp:123
 
   static const std::vector>
   SystemHeaderMap = {

ioeric wrote:
> hokein wrote:
> > sammccall wrote:
> > > hokein wrote:
> > > > ioeric wrote:
> > > > > Can we remove the suffix header mapping now? Is it for the 
> > > > > `std::chrono::` symbols? What are the reasons not to include them in 
> > > > > this patch? 
> > > > Ideally, we could remove it once we get a perfect symbol mapping but 
> > > > I'd prefer to keep it (it serves as a fallback when we don't find 
> > > > symbols in the symbol mapping), so that we could add new symbol mapping 
> > > > increasingly without changing the current behavior.
> > > > 
> > > > Removing it should be done carefully without introducing regression, 
> > > > and is outside the scope of this patch.
> > > We do need fallback heuristics. We should conisder cases:
> > >  - for known `std::` symbols mapped to one system header, we don't need a 
> > > fallback
> > >  - for known `std::` mapped to multiple system headers (`std::move`), we 
> > > need some fallback. There are probably few enough of these that it 
> > > doesn't justify listing *all* system header paths.
> > >  - for known `std::` symbols with a primary template in one file and 
> > > specializations/overloads in another (swap?) always inserting the primary 
> > > seems fine 
> > >  - For "random" unknown symbols from system header `foo/bits/_bar.h`, we 
> > > can not insert, correct to ``, or use the full path name. I don't 
> > > have a strong preference here, maybe we should do what's easiest.
> > >  - for c standard library (`::printf` --> `` etc) we probably 
> > > want mappings just like in this patch, I think cppreference has them.
> > >  - other "standardish" headers (POSIX etc) - hmm, unclear.
> > > 
> > > Thinking about all these cases, I'm thinking the nicest solution would be 
> > > having the symbol -> header mapping, having a small (symbol,header) -> 
> > > header mapping **only** for ambiguous cases, and not inserting "system" 
> > > headers that aren't in the main mapping at all. Then we can improve 
> > > coverage of posix, windows etc headers over time.
> > > Question is, how can we recognize "system" headers?
> > I think we were talking about C++ std symbols.
> > 
> > > for known std:: symbols mapped to one system header, we don't need a 
> > > fallback
> > > for known std:: mapped to multiple system headers (std::move), we need 
> > > some fallback. There are probably few enough of these that it doesn't 
> > > justify listing *all* system header paths.
> > > for known std:: symbols with a primary template in one file and 
> > > specializations/overloads in another (swap?) always inserting the primary 
> > > seems fine
> > 
> > As discussed in this patch, we have other ways to disambiguate these 
> > symbols (rather than relying on the header mapping), so it is possible to 
> > remove STL headers mapping, but we'll lose correct headers for STL 
> > implement-related symbols (e.g. `__hash_code`), ideally we should drop 
> > these symbols in the indexer...
> > 
> > > for c standard library (::printf -->  etc) we probably want 
> > > mappings just like in this patch, I think cppreference has them.
> > 
> > Yes, cppreference has them.
> > 
> > > other "standardish" headers (POSIX etc) - hmm, unclear.
> > 
> > I think we should still keep the header mapping. Not sure whether there are 
> > some sources like cppreference that list all symbols.
> > 
> > 
> So, do we have a plan on how to handle ambiguous symbols properly? If so, 
> could you please summarize this in the FIXME comment so that we don't lose 
> track of it?
> 
> > I think we should still keep the header mapping. Not sure whether there are 
> > some sources like cppreference that list all symbols.
> What's the reasoning?
> 
> I am +1 on Sam's idea about skipping include insertion for std:: 

[PATCH] D56370: [clangd] Add support for type hierarchy (super types only for now)

[Eric Liu via Phabricator via cfe-commits]

ioeric added a comment.

just a few drive-by comments ;)




Comment at: clang-tools-extra/clangd/FindSymbols.h:28
+// https://github.com/Microsoft/language-server-protocol/issues/344
+SymbolKind indexSymbolKindToSymbolKind(index::SymbolKind Kind);
+

This could probably live in `Protocol.h`.



Comment at: clang-tools-extra/clangd/XRefs.cpp:957
+  getTypeAncestors(*CXXRD, AST.getASTContext());
+  // TODO: Resolve type descendants if direction is Children or Both,
+  //   and ResolveLevels > 0.

nit: s/TODO/FIXME(owner)/

same elsewhere.



Comment at: clang-tools-extra/clangd/index/SymbolCollector.h:154
+// the SourceManager.
+std::string toURI(const SourceManager , llvm::StringRef Path,
+  llvm::StringRef FallbackDir);

why are we pulling this into the header? This still seems to be only used in 
SymbolCollector.cpp.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D56370



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

[PATCH] D58345: [clangd] Using symbol name to map includes for STL symbols.

[Eric Liu via Phabricator via cfe-commits]

ioeric added inline comments.



Comment at: clangd/StdGen/StdGen.py:2
+#!/usr/bin/env python
+#===- StdGen.py -  ---*- python 
-*--===#
+#

hokein wrote:
> ioeric wrote:
> > I'd avoid abbreviation in the file name and the new directory name. 
> > `StdGen` sounds a lot like a library ;)
> > 
> > I'd suggest something like `std-include-mapping/generate.py`
> Personally I'd vote `StdGen`, and it follows llvm's `TableGen`.  The name 
> `std-include-mapping/generate.py` seems too verbose...
Why do we want to follow `TableGen`?

Two reasons why I think a more informative name should be used. 1) `StdGen` 
sounds like a name for library and doesn't follow the naming convention for 
tool directory (e.g. `global-symbol-builder`) and 2) it's hard for readers 
without much context to understand what `StdGen` actually means.



Comment at: clangd/StdGen/StdGen.py:94
+  cpp_symbol_root = os.path.join(cpp_reference_root, "en", "cpp")
+  if not os.path.exists(cpp_reference_root):
+exit("Path %s doesn't exist!" % cpp_reference_root)

hokein wrote:
> ioeric wrote:
> > why not check `exists(index_page_path)` instead?
> I think either way works.
It seems to me that using `exists(index_page_path)` would be less likely to 
trigger an exception (better use experience). I don't see a good reason to 
check the root.



Comment at: clangd/StdSymbolMap.inc:8
+//===--===//
+// Used to build a lookup table (qualified names => include headers) for C++
+// Standard Library symbols.

hokein wrote:
> ioeric wrote:
> > can we include the information about the HTML archive (e.g. STL version) 
> > from which this table is generated? This would be useful for future 
> > maintenance.
> The version of HTML archive is the date information, but this information is 
> only present in the `.zip` name, we lose it after we unzip the `zip` file
If we can't get this from the doc, we should ask users to explicitly provide 
this information. I think we would want some version info to relate the output 
to the source.


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D58345



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

[PATCH] D58345: [clangd] Using symbol name to map includes for STL symbols.

[Eric Liu via Phabricator via cfe-commits]

ioeric added inline comments.



Comment at: clangd/StdGen/StdGen.py:2
+#!/usr/bin/env python
+#===- StdGen.py -  ---*- python 
-*--===#
+#

I'd avoid abbreviation in the file name and the new directory name. `StdGen` 
sounds a lot like a library ;)

I'd suggest something like `std-include-mapping/generate.py`



Comment at: clangd/StdGen/StdGen.py:14
+
+Caveats and FIXMEs:
+  - only symbols directly in "std::namespace" are added

we should also mention what the tool's behaviors are for these cases.



Comment at: clangd/StdGen/StdGen.py:15
+Caveats and FIXMEs:
+  - only symbols directly in "std::namespace" are added
+  - symbols with multiple variants, or defined in multiple headers aren't added

nit: s/"std::namespace"/std:: namespace/?



Comment at: clangd/StdGen/StdGen.py:16
+  - only symbols directly in "std::namespace" are added
+  - symbols with multiple variants, or defined in multiple headers aren't added
+

it's not trivial what these are. could provide an example here. 



Comment at: clangd/StdGen/StdGen.py:18
+
+Usage:
+  1. Install BeautifulSoup dependency

Suggestion: it would be nice to provide the common commands for each step. E.g. 
`pip install `. 



Comment at: clangd/StdGen/StdGen.py:23
+  3. Run the command:
+   StdGen.py -cppreference-doc  > StdGen.h
+"""

what is the `` here? is it the downloaded zip, the 
unzipped (e.g. `cppreference-doc-20181028/`), or some specific html doc in the 
directory?



Comment at: clangd/StdGen/StdGen.py:23
+  3. Run the command:
+   StdGen.py -cppreference-doc  > StdGen.h
+"""

ioeric wrote:
> what is the `` here? is it the downloaded zip, the 
> unzipped (e.g. `cppreference-doc-20181028/`), or some specific html doc in 
> the directory?
Again, `StdGen` is not great name. I'd suggest `StdHeaderMapping`.

nit: s/.h/.inc/



Comment at: clangd/StdGen/StdGen.py:32
+
+STDGEN_CODE = """\
+//===-- StdGen'erated file --*- C++ 
-*-===//

nit: maybe `STDGEN_CODE_HEADER`?



Comment at: clangd/StdGen/StdGen.py:48
+def ParseSymbolPage(symbol_page_html):
+  """Parse the symbol page and retrieve the include header defined in this 
page.
+  Returns a list of headers.

could you provide a sample html snippet in the comment? it would make the code 
easier to understand. thanks!

(same for `ParseIndexPage`)



Comment at: clangd/StdGen/StdGen.py:94
+  cpp_symbol_root = os.path.join(cpp_reference_root, "en", "cpp")
+  if not os.path.exists(cpp_reference_root):
+exit("Path %s doesn't exist!" % cpp_reference_root)

why not check `exists(index_page_path)` instead?



Comment at: clangd/StdGen/StdGen.py:116
+for name, headers in sorted(symbols.items(), key=lambda t : t[0]):
+  # FIXME: support ambiguous symbols
+  if len(headers) > 1:

move this FIXME into the `if` body to make it clearer that this is for 
`len(headers) > 1`. 

nit: it's not very clear whether `ambiguous symbols` = `symbols with multiple 
headers`. Maybe be more specific?



Comment at: clangd/StdSymbolMap.inc:8
+//===--===//
+// Used to build a lookup table (qualified names => include headers) for C++
+// Standard Library symbols.

can we include the information about the HTML archive (e.g. STL version) from 
which this table is generated? This would be useful for future maintenance.



Comment at: clangd/StdSymbolMap.inc:11
+//
+// Automatically generated file, do not edit.
+//===--===//

Add a pointer to the python script?



Comment at: clangd/index/CanonicalIncludes.cpp:123
 
   static const std::vector>
   SystemHeaderMap = {

hokein wrote:
> sammccall wrote:
> > hokein wrote:
> > > ioeric wrote:
> > > > Can we remove the suffix header mapping now? Is it for the 
> > > > `std::chrono::` symbols? What are the reasons not to include them in 
> > > > this patch? 
> > > Ideally, we could remove it once we get a perfect symbol mapping but I'd 
> > > prefer to keep it (it serves as a fallback when we don't find symbols in 
> > > the symbol mapping), so that we could add new symbol mapping increasingly 
> > > without changing the current behavior.
> > > 
> > > Removing it should be done carefully without introducing regression, and 
> > > is outside the scope of this patch.
> > We do need fallback heuristics. We should conisder cases:
> >  - for known `std::` symbols mapped to one system header, we don't need a 
> > fallback
> >  

[PATCH] D58977: [clang-tidy] Add the abseil-time-comparison check

[Eric Liu via Phabricator via cfe-commits]

ioeric accepted this revision.
ioeric added inline comments.
This revision is now accepted and ready to land.



Comment at: clang-tidy/abseil/TimeComparisonCheck.cpp:23
+  auto Matcher =
+  binaryOperator(anyOf(hasOperatorName(">"), hasOperatorName(">="),
+   hasOperatorName("=="), hasOperatorName("<="),

hwright wrote:
> ioeric wrote:
> > `DurationComparisonCheck.cpp` has a very similar matcher pattern.
> > 
> > Maybe pull out a common matcher for this? E.g. 
> > `comparisonOperatorWithCallee(...)`?
> > 
> My one concern about doing so is that it would move the name bindings into a 
> separate location away from the callback consuming those bindings.
> 
> Since this is only the second instance of this pattern, would it be 
> reasonable to wait until there's a third to combine them?
> My one concern about doing so is that it would move the name bindings into a 
> separate location away from the callback consuming those bindings.
I think the name binding can stay in the same file. 
`comparisonOperatorWithCallee` is a matcher for a FunctionDecl, so, on the call 
site, you could do 
`comparisonOperatorWithCallee(functionDecl(TimeConversionFunction()).bind("function_decl")).bind("binop")`.


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

https://reviews.llvm.org/D58977



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

[PATCH] D59086: [clangd] Adjust compile commands to be applicable for tooling

[Eric Liu via Phabricator via cfe-commits]

ioeric added inline comments.



Comment at: clangd/GlobalCompilationDatabase.cpp:25
+  // Clangd does not generate dependency file.
+  Cmd.CommandLine = tooling::getClangStripDependencyFileAdjuster()(
+  Cmd.CommandLine, Cmd.Filename);

Please use `clang::tooling::combineAdjusters`.


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D59086



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

[PATCH] D59086: [clangd] Adjust compile commands to be applicable for tooling

[Eric Liu via Phabricator via cfe-commits]

ioeric added inline comments.



Comment at: clangd/GlobalCompilationDatabase.h:116
+  /// Adjusts given compile command for clangd.
+  tooling::CompileCommand adjustArguments(tooling::CompileCommand Cmd) const;
+

This doesn't seem to be used in this patch (except for tests). Could you 
include intended uses in the patch so we can understand the problem better?


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D59086



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

[PATCH] D59083: [clangd] Store explicit template specializations in index for code navigation purposes

[Eric Liu via Phabricator via cfe-commits]

ioeric added a comment.

> We need that information for providing better code navigation support, like 
> find references, children/base classes etc.

It's not trivial how they make code navigation better. Can you explain and 
provide examples in the summary? Thanks!




Comment at: clangd/CodeComplete.cpp:1613
   };
+  // We index explicit template specializations merely for code navigation
+  // support.

nit: the context here is code completion. Maybe explain why they are not 
interesting for completion rather than why we index them?


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D59083



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

[PATCH] D58977: [clang-tidy] Add the abseil-time-comparison check

[Eric Liu via Phabricator via cfe-commits]

ioeric added inline comments.



Comment at: clang-tidy/abseil/TimeComparisonCheck.cpp:23
+  auto Matcher =
+  binaryOperator(anyOf(hasOperatorName(">"), hasOperatorName(">="),
+   hasOperatorName("=="), hasOperatorName("<="),

`DurationComparisonCheck.cpp` has a very similar matcher pattern.

Maybe pull out a common matcher for this? E.g. 
`comparisonOperatorWithCallee(...)`?




Comment at: clang-tidy/abseil/TimeComparisonCheck.cpp:42
+
+  // In most cases, we'll only need to rewrite one of the sides, but we also
+  // want to handle the case of rewriting both sides. This is much simpler if

Move this comment closer the replacement logic below?



Comment at: clang-tidy/abseil/TimeComparisonCheck.cpp:46
+  // if nothing needs to be done.
+  if (!isNotInMacro(Result, Binop->getLHS()) ||
+  !isNotInMacro(Result, Binop->getRHS()))

nit: double negation is a bit hard to read. maybe `!(isNotInMacro(LSH) && 
isNotInMacro(RHS))`? Ideally, the helper should be `isInMacro` instead of 
`isNotInMacro`.


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D58977



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

[PATCH] D58772: [clangd] Enable SuggestMissingIncludes by default.

[Eric Liu via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rCTE355200: [clangd] Enable SuggestMissingIncludes by default. 
(authored by ioeric, committed by ).

Changed prior to commit:
  https://reviews.llvm.org/D58772?vs=188701=188894#toc

Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D58772

Files:
  clangd/tool/ClangdMain.cpp


Index: clangd/tool/ClangdMain.cpp
===
--- clangd/tool/ClangdMain.cpp
+++ clangd/tool/ClangdMain.cpp
@@ -217,7 +217,7 @@
 "suggest-missing-includes",
 llvm::cl::desc("Attempts to fix diagnostic errors caused by missing "
"includes using index."),
-llvm::cl::init(false));
+llvm::cl::init(true));
 
 namespace {
 


Index: clangd/tool/ClangdMain.cpp
===
--- clangd/tool/ClangdMain.cpp
+++ clangd/tool/ClangdMain.cpp
@@ -217,7 +217,7 @@
 "suggest-missing-includes",
 llvm::cl::desc("Attempts to fix diagnostic errors caused by missing "
"includes using index."),
-llvm::cl::init(false));
+llvm::cl::init(true));
 
 namespace {
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D58782: Use ArrayRef::copy, instead of copying data manually

[Eric Liu via Phabricator via cfe-commits]

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

Nice!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D58782



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

[PATCH] D58781: Added missing license headers

[Eric Liu via Phabricator via cfe-commits]

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

lg. thanks!!!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D58781



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

[PATCH] D58778: Moved Ref into its own header and implementation file

[Eric Liu via Phabricator via cfe-commits]

ioeric added inline comments.



Comment at: clang-tools-extra/clangd/index/Ref.cpp:1
+#include "Ref.h"
+

License?



Comment at: clang-tools-extra/clangd/index/SymbolCollector.cpp:14
 #include "CodeCompletionStrings.h"
+#include "ExpectedTypes.h"
 #include "Logger.h"

Is this change relevant in this patch?



Comment at: clang-tools-extra/clangd/indexer/IndexerMain.cpp:16
 #include "index/Serialization.h"
+#include "index/Symbol.h"
 #include "index/SymbolCollector.h"

Maybe also include Ref.h?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D58778



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

[PATCH] D58774: Moved Symbol into its own header and implementation file

[Eric Liu via Phabricator via cfe-commits]

ioeric added inline comments.



Comment at: clang-tools-extra/clangd/index/Index.cpp:19
 
-llvm::raw_ostream <<(llvm::raw_ostream , Symbol::SymbolFlag F) {
-  if (F == Symbol::None)
-return OS << "None";
-  std::string S;
-  if (F & Symbol::Deprecated)
-S += "deprecated|";
-  if (F & Symbol::IndexedForCodeCompletion)
-S += "completion|";
-  return OS << llvm::StringRef(S).rtrim('|');
-}
-
-llvm::raw_ostream <<(llvm::raw_ostream , const Symbol ) {
-  return OS << S.Scope << S.Name;
-}
-
-float quality(const Symbol ) {
-  // This avoids a sharp gradient for tail symbols, and also neatly avoids the
-  // question of whether 0 references means a bad symbol or missing data.
-  if (S.References < 3)
-return 1;
-  return std::log(S.References);
-}
-
 SymbolSlab::const_iterator SymbolSlab::find(const SymbolID ) const {
   auto It = std::lower_bound(

should also move the implementation?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D58774



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

[PATCH] D58774: Moved Symbol into its own header and implementation file

[Eric Liu via Phabricator via cfe-commits]

ioeric added a comment.

The design of Symbol and SymbolSlab are closely related. I would suggest 
putting them close together in the same library.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D58774



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

[PATCH] D58773: Moved SymbolOrigin into its own header and implementation file

[Eric Liu via Phabricator via cfe-commits]

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

In D58773#1413486 , @gribozavr wrote:

> SymbolOrigin is used by itself, for example, in CodeComplete.h to define 
> `struct CodeCompletion`.


Fair enough. lgtm


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D58773



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

[PATCH] D58773: Moved SymbolOrigin into its own header and implementation file

[Eric Liu via Phabricator via cfe-commits]

ioeric added a comment.

I'm not sure if SymbolOrigin is interesting enough to be its own library. It's 
usually only used along with Symbol. Maybe we could pull a library Symbol.h 
instead and put SymbolOrigin by Symbol?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D58773



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

[PATCH] D58772: [clangd] Enable SuggestMissingIncludes by default.

[Eric Liu via Phabricator via cfe-commits]

ioeric created this revision.
ioeric added a reviewer: kadircet.
Herald added subscribers: cfe-commits, arphaman, jkorous, MaskRay, 
ilya-biryukov.
Herald added a project: clang.

This seems to work stably now. Turn on by default.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D58772

Files:
  clangd/tool/ClangdMain.cpp


Index: clangd/tool/ClangdMain.cpp
===
--- clangd/tool/ClangdMain.cpp
+++ clangd/tool/ClangdMain.cpp
@@ -217,7 +217,7 @@
 "suggest-missing-includes",
 llvm::cl::desc("Attempts to fix diagnostic errors caused by missing "
"includes using index."),
-llvm::cl::init(false));
+llvm::cl::init(true));
 
 namespace {
 


Index: clangd/tool/ClangdMain.cpp
===
--- clangd/tool/ClangdMain.cpp
+++ clangd/tool/ClangdMain.cpp
@@ -217,7 +217,7 @@
 "suggest-missing-includes",
 llvm::cl::desc("Attempts to fix diagnostic errors caused by missing "
"includes using index."),
-llvm::cl::init(false));
+llvm::cl::init(true));
 
 namespace {
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D56830: Prototype for include-fixer workflow in clangd. [NOT FOR REVIEW]

[Eric Liu via Phabricator via cfe-commits]

ioeric abandoned this revision.
ioeric added a comment.
Herald added a subscriber: jdoerfert.
Herald added a project: clang.

Include-fixer has been landed in clangd.


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D56830



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

[PATCH] D58768: Moved SymbolLocation into its own header and implementation file

[Eric Liu via Phabricator via cfe-commits]

ioeric added a comment.

I think another option here is to move Symbol+SymbolLocations to a Symbol.h 
library. SymbolLocation is often used along with Symbol.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D58768



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

[PATCH] D58448: [clangd] Improve global code completion when scope specifier is unresolved.

[Eric Liu via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL354963: [clangd] Improve global code completion when scope 
specifier is unresolved. (authored by ioeric, committed by ).
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.

Repository:
  rL LLVM

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

https://reviews.llvm.org/D58448

Files:
  clang-tools-extra/trunk/clangd/CodeComplete.cpp
  clang-tools-extra/trunk/unittests/clangd/CodeCompleteTests.cpp


Index: clang-tools-extra/trunk/unittests/clangd/CodeCompleteTests.cpp
===
--- clang-tools-extra/trunk/unittests/clangd/CodeCompleteTests.cpp
+++ clang-tools-extra/trunk/unittests/clangd/CodeCompleteTests.cpp
@@ -1095,8 +1095,10 @@
   } // namespace ns
   )cpp");
 
-  EXPECT_THAT(Requests, ElementsAre(Field(::Scopes,
-  UnorderedElementsAre("bar::";
+  EXPECT_THAT(Requests,
+  ElementsAre(Field(
+  ::Scopes,
+  UnorderedElementsAre("a::bar::", "ns::bar::", "bar::";
 }
 
 TEST(CompletionTest, UnresolvedNestedQualifierIdQuery) {
@@ -2335,6 +2337,35 @@
 Kind(CompletionItemKind::Reference;
 }
 
+TEST(CompletionTest, ScopeIsUnresolved) {
+  clangd::CodeCompleteOptions Opts = {};
+  Opts.AllScopes = true;
+
+  auto Results = completions(R"cpp(
+namespace a {
+void f() { b::X^ }
+}
+  )cpp",
+ {cls("a::b::XYZ")}, Opts);
+  EXPECT_THAT(Results.Completions,
+  UnorderedElementsAre(AllOf(Qualifier(""), Named("XYZ";
+}
+
+TEST(CompletionTest, NestedScopeIsUnresolved) {
+  clangd::CodeCompleteOptions Opts = {};
+  Opts.AllScopes = true;
+
+  auto Results = completions(R"cpp(
+namespace a {
+namespace b {}
+void f() { b::c::X^ }
+}
+  )cpp",
+ {cls("a::b::c::XYZ")}, Opts);
+  EXPECT_THAT(Results.Completions,
+  UnorderedElementsAre(AllOf(Qualifier(""), Named("XYZ";
+}
+
 } // namespace
 } // namespace clangd
 } // namespace clang
Index: clang-tools-extra/trunk/clangd/CodeComplete.cpp
===
--- clang-tools-extra/trunk/clangd/CodeComplete.cpp
+++ clang-tools-extra/trunk/clangd/CodeComplete.cpp
@@ -48,6 +48,7 @@
 #include "llvm/ADT/None.h"
 #include "llvm/ADT/Optional.h"
 #include "llvm/ADT/SmallVector.h"
+#include "llvm/ADT/StringRef.h"
 #include "llvm/Support/Error.h"
 #include "llvm/Support/Format.h"
 #include "llvm/Support/FormatVariadic.h"
@@ -526,7 +527,7 @@
 std::set Results;
 for (llvm::StringRef AS : AccessibleScopes)
   Results.insert(
-  ((UnresolvedQualifier ? *UnresolvedQualifier : "") + AS).str());
+  (AS + (UnresolvedQualifier ? *UnresolvedQualifier : "")).str());
 return {Results.begin(), Results.end()};
   }
 };
@@ -570,16 +571,15 @@
   }
 
   // Unresolved qualifier.
-  // FIXME: When Sema can resolve part of a scope chain (e.g.
-  // "known::unknown::id"), we should expand the known part ("known::") rather
-  // than treating the whole thing as unknown.
-  SpecifiedScope Info;
-  Info.AccessibleScopes.push_back(""); // global namespace
+  SpecifiedScope Info = GetAllAccessibleScopes(CCContext);
+  Info.AccessibleScopes.push_back(""); // Make sure global scope is included.
 
-  Info.UnresolvedQualifier =
+  llvm::StringRef SpelledSpecifier =
   Lexer::getSourceText(CharSourceRange::getCharRange((*SS)->getRange()),
-   CCSema.SourceMgr, clang::LangOptions())
-  .ltrim("::");
+   CCSema.SourceMgr, clang::LangOptions());
+  if (SpelledSpecifier.consume_front("::"))
+Info.AccessibleScopes = {""};
+  Info.UnresolvedQualifier = SpelledSpecifier;
   // Sema excludes the trailing "::".
   if (!Info.UnresolvedQualifier->empty())
 *Info.UnresolvedQualifier += "::";


Index: clang-tools-extra/trunk/unittests/clangd/CodeCompleteTests.cpp
===
--- clang-tools-extra/trunk/unittests/clangd/CodeCompleteTests.cpp
+++ clang-tools-extra/trunk/unittests/clangd/CodeCompleteTests.cpp
@@ -1095,8 +1095,10 @@
   } // namespace ns
   )cpp");
 
-  EXPECT_THAT(Requests, ElementsAre(Field(::Scopes,
-  UnorderedElementsAre("bar::";
+  EXPECT_THAT(Requests,
+  ElementsAre(Field(
+  ::Scopes,
+  UnorderedElementsAre("a::bar::", "ns::bar::", "bar::";
 }
 
 TEST(CompletionTest, UnresolvedNestedQualifierIdQuery) {
@@ -2335,6 +2337,35 @@
 Kind(CompletionItemKind::Reference;
 }
 
+TEST(CompletionTest, ScopeIsUnresolved) {
+  clangd::CodeCompleteOptions Opts = {};
+  Opts.AllScopes = true;
+
+  auto Results = completions(R"cpp(
+namespace a {
+void f() { 

[PATCH] D58448: [clangd] Improve global code completion when scope specifier is unresolved.

[Eric Liu via Phabricator via cfe-commits]

ioeric updated this revision to Diff 188512.
ioeric added a comment.

- Rebase and minor cleanup


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D58448

Files:
  clangd/CodeComplete.cpp
  unittests/clangd/CodeCompleteTests.cpp


Index: unittests/clangd/CodeCompleteTests.cpp
===
--- unittests/clangd/CodeCompleteTests.cpp
+++ unittests/clangd/CodeCompleteTests.cpp
@@ -1095,8 +1095,10 @@
   } // namespace ns
   )cpp");
 
-  EXPECT_THAT(Requests, ElementsAre(Field(::Scopes,
-  UnorderedElementsAre("bar::";
+  EXPECT_THAT(Requests,
+  ElementsAre(Field(
+  ::Scopes,
+  UnorderedElementsAre("a::bar::", "ns::bar::", "bar::";
 }
 
 TEST(CompletionTest, UnresolvedNestedQualifierIdQuery) {
@@ -2335,6 +2337,35 @@
 Kind(CompletionItemKind::Reference;
 }
 
+TEST(CompletionTest, ScopeIsUnresolved) {
+  clangd::CodeCompleteOptions Opts = {};
+  Opts.AllScopes = true;
+
+  auto Results = completions(R"cpp(
+namespace a {
+void f() { b::X^ }
+}
+  )cpp",
+ {cls("a::b::XYZ")}, Opts);
+  EXPECT_THAT(Results.Completions,
+  UnorderedElementsAre(AllOf(Qualifier(""), Named("XYZ";
+}
+
+TEST(CompletionTest, NestedScopeIsUnresolved) {
+  clangd::CodeCompleteOptions Opts = {};
+  Opts.AllScopes = true;
+
+  auto Results = completions(R"cpp(
+namespace a {
+namespace b {}
+void f() { b::c::X^ }
+}
+  )cpp",
+ {cls("a::b::c::XYZ")}, Opts);
+  EXPECT_THAT(Results.Completions,
+  UnorderedElementsAre(AllOf(Qualifier(""), Named("XYZ";
+}
+
 } // namespace
 } // namespace clangd
 } // namespace clang
Index: clangd/CodeComplete.cpp
===
--- clangd/CodeComplete.cpp
+++ clangd/CodeComplete.cpp
@@ -48,6 +48,7 @@
 #include "llvm/ADT/None.h"
 #include "llvm/ADT/Optional.h"
 #include "llvm/ADT/SmallVector.h"
+#include "llvm/ADT/StringRef.h"
 #include "llvm/Support/Error.h"
 #include "llvm/Support/Format.h"
 #include "llvm/Support/FormatVariadic.h"
@@ -526,7 +527,7 @@
 std::set Results;
 for (llvm::StringRef AS : AccessibleScopes)
   Results.insert(
-  ((UnresolvedQualifier ? *UnresolvedQualifier : "") + AS).str());
+  (AS + (UnresolvedQualifier ? *UnresolvedQualifier : "")).str());
 return {Results.begin(), Results.end()};
   }
 };
@@ -570,16 +571,15 @@
   }
 
   // Unresolved qualifier.
-  // FIXME: When Sema can resolve part of a scope chain (e.g.
-  // "known::unknown::id"), we should expand the known part ("known::") rather
-  // than treating the whole thing as unknown.
-  SpecifiedScope Info;
-  Info.AccessibleScopes.push_back(""); // global namespace
+  SpecifiedScope Info = GetAllAccessibleScopes(CCContext);
+  Info.AccessibleScopes.push_back(""); // Make sure global scope is included.
 
-  Info.UnresolvedQualifier =
+  llvm::StringRef SpelledSpecifier =
   Lexer::getSourceText(CharSourceRange::getCharRange((*SS)->getRange()),
-   CCSema.SourceMgr, clang::LangOptions())
-  .ltrim("::");
+   CCSema.SourceMgr, clang::LangOptions());
+  if (SpelledSpecifier.consume_front("::"))
+Info.AccessibleScopes = {""};
+  Info.UnresolvedQualifier = SpelledSpecifier;
   // Sema excludes the trailing "::".
   if (!Info.UnresolvedQualifier->empty())
 *Info.UnresolvedQualifier += "::";


Index: unittests/clangd/CodeCompleteTests.cpp
===
--- unittests/clangd/CodeCompleteTests.cpp
+++ unittests/clangd/CodeCompleteTests.cpp
@@ -1095,8 +1095,10 @@
   } // namespace ns
   )cpp");
 
-  EXPECT_THAT(Requests, ElementsAre(Field(::Scopes,
-  UnorderedElementsAre("bar::";
+  EXPECT_THAT(Requests,
+  ElementsAre(Field(
+  ::Scopes,
+  UnorderedElementsAre("a::bar::", "ns::bar::", "bar::";
 }
 
 TEST(CompletionTest, UnresolvedNestedQualifierIdQuery) {
@@ -2335,6 +2337,35 @@
 Kind(CompletionItemKind::Reference;
 }
 
+TEST(CompletionTest, ScopeIsUnresolved) {
+  clangd::CodeCompleteOptions Opts = {};
+  Opts.AllScopes = true;
+
+  auto Results = completions(R"cpp(
+namespace a {
+void f() { b::X^ }
+}
+  )cpp",
+ {cls("a::b::XYZ")}, Opts);
+  EXPECT_THAT(Results.Completions,
+  UnorderedElementsAre(AllOf(Qualifier(""), Named("XYZ";
+}
+
+TEST(CompletionTest, NestedScopeIsUnresolved) {
+  clangd::CodeCompleteOptions Opts = {};
+  Opts.AllScopes = true;
+
+  auto Results = completions(R"cpp(
+namespace a {
+namespace b {}
+void f() { b::c::X^ }
+}
+  )cpp",
+ 

[PATCH] D58446: [CodeComplete] Collect visited contexts when scope specifier is invalid.

[Eric Liu via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL354570: [CodeComplete] Collect visited contexts when scope 
specifier is invalid. (authored by ioeric, committed by ).
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.

Repository:
  rL LLVM

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

https://reviews.llvm.org/D58446

Files:
  cfe/trunk/lib/Sema/SemaCodeComplete.cpp
  cfe/trunk/unittests/Sema/CodeCompleteTest.cpp


Index: cfe/trunk/lib/Sema/SemaCodeComplete.cpp
===
--- cfe/trunk/lib/Sema/SemaCodeComplete.cpp
+++ cfe/trunk/lib/Sema/SemaCodeComplete.cpp
@@ -5061,7 +5061,20 @@
   if (SS.isInvalid()) {
 CodeCompletionContext CC(CodeCompletionContext::CCC_Symbol);
 CC.setCXXScopeSpecifier(SS);
-HandleCodeCompleteResults(this, CodeCompleter, CC, nullptr, 0);
+// As SS is invalid, we try to collect accessible contexts from the current
+// scope with a dummy lookup so that the completion consumer can try to
+// guess what the specified scope is.
+ResultBuilder DummyResults(*this, CodeCompleter->getAllocator(),
+   CodeCompleter->getCodeCompletionTUInfo(), CC);
+if (S->getEntity()) {
+  CodeCompletionDeclConsumer Consumer(DummyResults, S->getEntity(),
+  BaseType);
+  LookupVisibleDecls(S, LookupOrdinaryName, Consumer,
+ /*IncludeGlobalScope=*/false,
+ /*LoadExternal=*/false);
+}
+HandleCodeCompleteResults(this, CodeCompleter,
+  DummyResults.getCompletionContext(), nullptr, 0);
 return;
   }
   // Always pretend to enter a context to ensure that a dependent type
Index: cfe/trunk/unittests/Sema/CodeCompleteTest.cpp
===
--- cfe/trunk/unittests/Sema/CodeCompleteTest.cpp
+++ cfe/trunk/unittests/Sema/CodeCompleteTest.cpp
@@ -173,12 +173,16 @@
   "foo::(anonymous)"));
 }
 
-TEST(SemaCodeCompleteTest, VisitedNSForInvalideQualifiedId) {
+TEST(SemaCodeCompleteTest, VisitedNSForInvalidQualifiedId) {
   auto VisitedNS = runCodeCompleteOnCode(R"cpp(
- namespace ns { foo::^ }
+ namespace na {}
+ namespace ns1 {
+ using namespace na;
+ foo::^
+ }
   )cpp")
.VisitedNamespaces;
-  EXPECT_TRUE(VisitedNS.empty());
+  EXPECT_THAT(VisitedNS, UnorderedElementsAre("ns1", "na"));
 }
 
 TEST(SemaCodeCompleteTest, VisitedNSWithoutQualifier) {


Index: cfe/trunk/lib/Sema/SemaCodeComplete.cpp
===
--- cfe/trunk/lib/Sema/SemaCodeComplete.cpp
+++ cfe/trunk/lib/Sema/SemaCodeComplete.cpp
@@ -5061,7 +5061,20 @@
   if (SS.isInvalid()) {
 CodeCompletionContext CC(CodeCompletionContext::CCC_Symbol);
 CC.setCXXScopeSpecifier(SS);
-HandleCodeCompleteResults(this, CodeCompleter, CC, nullptr, 0);
+// As SS is invalid, we try to collect accessible contexts from the current
+// scope with a dummy lookup so that the completion consumer can try to
+// guess what the specified scope is.
+ResultBuilder DummyResults(*this, CodeCompleter->getAllocator(),
+   CodeCompleter->getCodeCompletionTUInfo(), CC);
+if (S->getEntity()) {
+  CodeCompletionDeclConsumer Consumer(DummyResults, S->getEntity(),
+  BaseType);
+  LookupVisibleDecls(S, LookupOrdinaryName, Consumer,
+ /*IncludeGlobalScope=*/false,
+ /*LoadExternal=*/false);
+}
+HandleCodeCompleteResults(this, CodeCompleter,
+  DummyResults.getCompletionContext(), nullptr, 0);
 return;
   }
   // Always pretend to enter a context to ensure that a dependent type
Index: cfe/trunk/unittests/Sema/CodeCompleteTest.cpp
===
--- cfe/trunk/unittests/Sema/CodeCompleteTest.cpp
+++ cfe/trunk/unittests/Sema/CodeCompleteTest.cpp
@@ -173,12 +173,16 @@
   "foo::(anonymous)"));
 }
 
-TEST(SemaCodeCompleteTest, VisitedNSForInvalideQualifiedId) {
+TEST(SemaCodeCompleteTest, VisitedNSForInvalidQualifiedId) {
   auto VisitedNS = runCodeCompleteOnCode(R"cpp(
- namespace ns { foo::^ }
+ namespace na {}
+ namespace ns1 {
+ using namespace na;
+ foo::^
+ }
   )cpp")
.VisitedNamespaces;
-  EXPECT_TRUE(VisitedNS.empty());
+  EXPECT_THAT(VisitedNS, UnorderedElementsAre("ns1", "na"));
 }
 
 TEST(SemaCodeCompleteTest, VisitedNSWithoutQualifier) {
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D58446: [CodeComplete] Collect visited contexts when scope specifier is invalid.

[Eric Liu via Phabricator via cfe-commits]

ioeric updated this revision to Diff 187756.
ioeric added a comment.

- Cleanup unit tests (forgot to run... sorry)


Repository:
  rC Clang

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

https://reviews.llvm.org/D58446

Files:
  lib/Sema/SemaCodeComplete.cpp
  unittests/Sema/CodeCompleteTest.cpp


Index: unittests/Sema/CodeCompleteTest.cpp
===
--- unittests/Sema/CodeCompleteTest.cpp
+++ unittests/Sema/CodeCompleteTest.cpp
@@ -173,12 +173,16 @@
   "foo::(anonymous)"));
 }
 
-TEST(SemaCodeCompleteTest, VisitedNSForInvalideQualifiedId) {
+TEST(SemaCodeCompleteTest, VisitedNSForInvalidQualifiedId) {
   auto VisitedNS = runCodeCompleteOnCode(R"cpp(
- namespace ns { foo::^ }
+ namespace na {}
+ namespace ns1 {
+ using namespace na;
+ foo::^
+ }
   )cpp")
.VisitedNamespaces;
-  EXPECT_TRUE(VisitedNS.empty());
+  EXPECT_THAT(VisitedNS, UnorderedElementsAre("ns1", "na"));
 }
 
 TEST(SemaCodeCompleteTest, VisitedNSWithoutQualifier) {
Index: lib/Sema/SemaCodeComplete.cpp
===
--- lib/Sema/SemaCodeComplete.cpp
+++ lib/Sema/SemaCodeComplete.cpp
@@ -5061,7 +5061,20 @@
   if (SS.isInvalid()) {
 CodeCompletionContext CC(CodeCompletionContext::CCC_Symbol);
 CC.setCXXScopeSpecifier(SS);
-HandleCodeCompleteResults(this, CodeCompleter, CC, nullptr, 0);
+// As SS is invalid, we try to collect accessible contexts from the current
+// scope with a dummy lookup so that the completion consumer can try to
+// guess what the specified scope is.
+ResultBuilder DummyResults(*this, CodeCompleter->getAllocator(),
+   CodeCompleter->getCodeCompletionTUInfo(), CC);
+if (S->getEntity()) {
+  CodeCompletionDeclConsumer Consumer(DummyResults, S->getEntity(),
+  BaseType);
+  LookupVisibleDecls(S, LookupOrdinaryName, Consumer,
+ /*IncludeGlobalScope=*/false,
+ /*LoadExternal=*/false);
+}
+HandleCodeCompleteResults(this, CodeCompleter,
+  DummyResults.getCompletionContext(), nullptr, 0);
 return;
   }
   // Always pretend to enter a context to ensure that a dependent type


Index: unittests/Sema/CodeCompleteTest.cpp
===
--- unittests/Sema/CodeCompleteTest.cpp
+++ unittests/Sema/CodeCompleteTest.cpp
@@ -173,12 +173,16 @@
   "foo::(anonymous)"));
 }
 
-TEST(SemaCodeCompleteTest, VisitedNSForInvalideQualifiedId) {
+TEST(SemaCodeCompleteTest, VisitedNSForInvalidQualifiedId) {
   auto VisitedNS = runCodeCompleteOnCode(R"cpp(
- namespace ns { foo::^ }
+ namespace na {}
+ namespace ns1 {
+ using namespace na;
+ foo::^
+ }
   )cpp")
.VisitedNamespaces;
-  EXPECT_TRUE(VisitedNS.empty());
+  EXPECT_THAT(VisitedNS, UnorderedElementsAre("ns1", "na"));
 }
 
 TEST(SemaCodeCompleteTest, VisitedNSWithoutQualifier) {
Index: lib/Sema/SemaCodeComplete.cpp
===
--- lib/Sema/SemaCodeComplete.cpp
+++ lib/Sema/SemaCodeComplete.cpp
@@ -5061,7 +5061,20 @@
   if (SS.isInvalid()) {
 CodeCompletionContext CC(CodeCompletionContext::CCC_Symbol);
 CC.setCXXScopeSpecifier(SS);
-HandleCodeCompleteResults(this, CodeCompleter, CC, nullptr, 0);
+// As SS is invalid, we try to collect accessible contexts from the current
+// scope with a dummy lookup so that the completion consumer can try to
+// guess what the specified scope is.
+ResultBuilder DummyResults(*this, CodeCompleter->getAllocator(),
+   CodeCompleter->getCodeCompletionTUInfo(), CC);
+if (S->getEntity()) {
+  CodeCompletionDeclConsumer Consumer(DummyResults, S->getEntity(),
+  BaseType);
+  LookupVisibleDecls(S, LookupOrdinaryName, Consumer,
+ /*IncludeGlobalScope=*/false,
+ /*LoadExternal=*/false);
+}
+HandleCodeCompleteResults(this, CodeCompleter,
+  DummyResults.getCompletionContext(), nullptr, 0);
 return;
   }
   // Always pretend to enter a context to ensure that a dependent type
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D58448: [clangd] Improve global code completion when scope specifier is unresolved.

[Eric Liu via Phabricator via cfe-commits]

ioeric created this revision.
ioeric added reviewers: ilya-biryukov, sammccall.
Herald added subscribers: cfe-commits, jdoerfert, kadircet, arphaman, jkorous, 
MaskRay.
Herald added a project: clang.

Suppose `clangd::` is unresolved in the following example. Currently,
we simply use "clangd::" as the query scope. We can do better by combining with
accessible scopes in the context. The query scopes can be `{clangd::, 
clang::clangd::}`.

  namespace clang { clangd::^ }


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D58448

Files:
  clangd/CodeComplete.cpp
  unittests/clangd/CodeCompleteTests.cpp


Index: unittests/clangd/CodeCompleteTests.cpp
===
--- unittests/clangd/CodeCompleteTests.cpp
+++ unittests/clangd/CodeCompleteTests.cpp
@@ -1094,8 +1094,10 @@
   } // namespace ns
   )cpp");
 
-  EXPECT_THAT(Requests, ElementsAre(Field(::Scopes,
-  UnorderedElementsAre("bar::";
+  EXPECT_THAT(Requests,
+  ElementsAre(Field(
+  ::Scopes,
+  UnorderedElementsAre("a::bar::", "ns::bar::", "bar::";
 }
 
 TEST(CompletionTest, UnresolvedNestedQualifierIdQuery) {
@@ -2314,6 +2316,35 @@
   EXPECT_THAT(R.Completions, ElementsAre(Named("loopVar")));
 }
 
+TEST(CompletionTest, ScopeIsUnresolved) {
+  clangd::CodeCompleteOptions Opts = {};
+  Opts.AllScopes = true;
+
+  auto Results = completions(R"cpp(
+namespace a {
+void f() { b::X^ }
+}
+  )cpp",
+ {cls("a::b::XYZ")}, Opts);
+  EXPECT_THAT(Results.Completions,
+  UnorderedElementsAre(AllOf(Qualifier(""), Named("XYZ";
+}
+
+TEST(CompletionTest, NestedScopeIsUnresolved) {
+  clangd::CodeCompleteOptions Opts = {};
+  Opts.AllScopes = true;
+
+  auto Results = completions(R"cpp(
+namespace a {
+namespace b {}
+void f() { b::c::X^ }
+}
+  )cpp",
+ {cls("a::b::c::XYZ")}, Opts);
+  EXPECT_THAT(Results.Completions,
+  UnorderedElementsAre(AllOf(Qualifier(""), Named("XYZ";
+}
+
 } // namespace
 } // namespace clangd
 } // namespace clang
Index: clangd/CodeComplete.cpp
===
--- clangd/CodeComplete.cpp
+++ clangd/CodeComplete.cpp
@@ -48,6 +48,7 @@
 #include "llvm/ADT/None.h"
 #include "llvm/ADT/Optional.h"
 #include "llvm/ADT/SmallVector.h"
+#include "llvm/ADT/StringRef.h"
 #include "llvm/Support/Error.h"
 #include "llvm/Support/Format.h"
 #include "llvm/Support/FormatVariadic.h"
@@ -526,7 +527,7 @@
 std::set Results;
 for (llvm::StringRef AS : AccessibleScopes)
   Results.insert(
-  ((UnresolvedQualifier ? *UnresolvedQualifier : "") + AS).str());
+  (AS + (UnresolvedQualifier ? *UnresolvedQualifier : "")).str());
 return {Results.begin(), Results.end()};
   }
 };
@@ -570,16 +571,16 @@
   }
 
   // Unresolved qualifier.
-  // FIXME: When Sema can resolve part of a scope chain (e.g.
-  // "known::unknown::id"), we should expand the known part ("known::") rather
-  // than treating the whole thing as unknown.
-  SpecifiedScope Info;
-  Info.AccessibleScopes.push_back(""); // global namespace
+  SpecifiedScope Info = GetAllAccessibleScopes(CCContext);
+  if (Info.AccessibleScopes.empty())
+Info.AccessibleScopes.push_back(""); // Fallback to global namespace.
 
-  Info.UnresolvedQualifier =
+  llvm::StringRef SpelledSpecifier =
   Lexer::getSourceText(CharSourceRange::getCharRange((*SS)->getRange()),
-   CCSema.SourceMgr, clang::LangOptions())
-  .ltrim("::");
+   CCSema.SourceMgr, clang::LangOptions());
+  if (SpelledSpecifier.consume_front("::"))
+Info.AccessibleScopes = {""};
+  Info.UnresolvedQualifier = SpelledSpecifier;
   // Sema excludes the trailing "::".
   if (!Info.UnresolvedQualifier->empty())
 *Info.UnresolvedQualifier += "::";


Index: unittests/clangd/CodeCompleteTests.cpp
===
--- unittests/clangd/CodeCompleteTests.cpp
+++ unittests/clangd/CodeCompleteTests.cpp
@@ -1094,8 +1094,10 @@
   } // namespace ns
   )cpp");
 
-  EXPECT_THAT(Requests, ElementsAre(Field(::Scopes,
-  UnorderedElementsAre("bar::";
+  EXPECT_THAT(Requests,
+  ElementsAre(Field(
+  ::Scopes,
+  UnorderedElementsAre("a::bar::", "ns::bar::", "bar::";
 }
 
 TEST(CompletionTest, UnresolvedNestedQualifierIdQuery) {
@@ -2314,6 +2316,35 @@
   EXPECT_THAT(R.Completions, ElementsAre(Named("loopVar")));
 }
 
+TEST(CompletionTest, ScopeIsUnresolved) {
+  clangd::CodeCompleteOptions Opts = {};
+  Opts.AllScopes = true;
+
+  auto Results = completions(R"cpp(
+namespace a {
+void f() { b::X^ }
+}
+  )cpp",
+ {cls("a::b::XYZ")}, Opts);
+  

[PATCH] D58446: [CodeComplete] Collect visited contexts when scope specifier is invalid.

[Eric Liu via Phabricator via cfe-commits]

ioeric created this revision.
ioeric added a reviewer: ilya-biryukov.
Herald added subscribers: cfe-commits, jdoerfert.
Herald added a project: clang.

This will allow completion consumers to guess the specified scope by
putting together scopes in the context with the specified scope (e.g. when the
specified namespace is not imported yet).


Repository:
  rC Clang

https://reviews.llvm.org/D58446

Files:
  lib/Sema/SemaCodeComplete.cpp
  unittests/Sema/CodeCompleteTest.cpp


Index: unittests/Sema/CodeCompleteTest.cpp
===
--- unittests/Sema/CodeCompleteTest.cpp
+++ unittests/Sema/CodeCompleteTest.cpp
@@ -181,6 +181,18 @@
   EXPECT_TRUE(VisitedNS.empty());
 }
 
+TEST(SemaCodeCompleteTest, VisitedNSForInvalidQualifiedId) {
+  auto VisitedNS = runCodeCompleteOnCode(R"cpp(
+ namespace na {}
+ namespace ns1 {
+ using namespace na;
+ foo::^
+ }
+  )cpp")
+   .VisitedNamespaces;
+  EXPECT_THAT(VisitedNS, UnorderedElementsAre("ns1::", "na::"));
+}
+
 TEST(SemaCodeCompleteTest, VisitedNSWithoutQualifier) {
   auto VisitedNS = runCodeCompleteOnCode(R"cpp(
 namespace n1 {
Index: lib/Sema/SemaCodeComplete.cpp
===
--- lib/Sema/SemaCodeComplete.cpp
+++ lib/Sema/SemaCodeComplete.cpp
@@ -5061,7 +5061,20 @@
   if (SS.isInvalid()) {
 CodeCompletionContext CC(CodeCompletionContext::CCC_Symbol);
 CC.setCXXScopeSpecifier(SS);
-HandleCodeCompleteResults(this, CodeCompleter, CC, nullptr, 0);
+// As SS is invalid, we try to collect accessible contexts from the current
+// scope with a dummy lookup so that the completion consumer can try to
+// guess what the specified scope is.
+ResultBuilder DummyResults(*this, CodeCompleter->getAllocator(),
+   CodeCompleter->getCodeCompletionTUInfo(), CC);
+if (S->getEntity()) {
+  CodeCompletionDeclConsumer Consumer(DummyResults, S->getEntity(),
+  BaseType);
+  LookupVisibleDecls(S, LookupOrdinaryName, Consumer,
+ /*IncludeGlobalScope=*/false,
+ /*LoadExternal=*/false);
+}
+HandleCodeCompleteResults(this, CodeCompleter,
+  DummyResults.getCompletionContext(), nullptr, 0);
 return;
   }
   // Always pretend to enter a context to ensure that a dependent type


Index: unittests/Sema/CodeCompleteTest.cpp
===
--- unittests/Sema/CodeCompleteTest.cpp
+++ unittests/Sema/CodeCompleteTest.cpp
@@ -181,6 +181,18 @@
   EXPECT_TRUE(VisitedNS.empty());
 }
 
+TEST(SemaCodeCompleteTest, VisitedNSForInvalidQualifiedId) {
+  auto VisitedNS = runCodeCompleteOnCode(R"cpp(
+ namespace na {}
+ namespace ns1 {
+ using namespace na;
+ foo::^
+ }
+  )cpp")
+   .VisitedNamespaces;
+  EXPECT_THAT(VisitedNS, UnorderedElementsAre("ns1::", "na::"));
+}
+
 TEST(SemaCodeCompleteTest, VisitedNSWithoutQualifier) {
   auto VisitedNS = runCodeCompleteOnCode(R"cpp(
 namespace n1 {
Index: lib/Sema/SemaCodeComplete.cpp
===
--- lib/Sema/SemaCodeComplete.cpp
+++ lib/Sema/SemaCodeComplete.cpp
@@ -5061,7 +5061,20 @@
   if (SS.isInvalid()) {
 CodeCompletionContext CC(CodeCompletionContext::CCC_Symbol);
 CC.setCXXScopeSpecifier(SS);
-HandleCodeCompleteResults(this, CodeCompleter, CC, nullptr, 0);
+// As SS is invalid, we try to collect accessible contexts from the current
+// scope with a dummy lookup so that the completion consumer can try to
+// guess what the specified scope is.
+ResultBuilder DummyResults(*this, CodeCompleter->getAllocator(),
+   CodeCompleter->getCodeCompletionTUInfo(), CC);
+if (S->getEntity()) {
+  CodeCompletionDeclConsumer Consumer(DummyResults, S->getEntity(),
+  BaseType);
+  LookupVisibleDecls(S, LookupOrdinaryName, Consumer,
+ /*IncludeGlobalScope=*/false,
+ /*LoadExternal=*/false);
+}
+HandleCodeCompleteResults(this, CodeCompleter,
+  DummyResults.getCompletionContext(), nullptr, 0);
 return;
   }
   // Always pretend to enter a context to ensure that a dependent type
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D58185: [clangd] Handle unresolved scope specifier when fixing includes.

[Eric Liu via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL354330: [clangd] Handle unresolved scope specifier when 
fixing includes. (authored by ioeric, committed by ).
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.

Repository:
  rL LLVM

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

https://reviews.llvm.org/D58185

Files:
  clang-tools-extra/trunk/clangd/IncludeFixer.cpp
  clang-tools-extra/trunk/unittests/clangd/DiagnosticsTests.cpp

Index: clang-tools-extra/trunk/clangd/IncludeFixer.cpp
===
--- clang-tools-extra/trunk/clangd/IncludeFixer.cpp
+++ clang-tools-extra/trunk/clangd/IncludeFixer.cpp
@@ -19,6 +19,10 @@
 #include "clang/AST/Type.h"
 #include "clang/Basic/Diagnostic.h"
 #include "clang/Basic/DiagnosticSema.h"
+#include "clang/Basic/LangOptions.h"
+#include "clang/Basic/SourceLocation.h"
+#include "clang/Basic/SourceManager.h"
+#include "clang/Basic/TokenKinds.h"
 #include "clang/Sema/DeclSpec.h"
 #include "clang/Sema/Lookup.h"
 #include "clang/Sema/Scope.h"
@@ -28,6 +32,7 @@
 #include "llvm/ADT/DenseMap.h"
 #include "llvm/ADT/None.h"
 #include "llvm/ADT/Optional.h"
+#include "llvm/ADT/StringRef.h"
 #include "llvm/ADT/StringSet.h"
 #include "llvm/Support/Error.h"
 #include "llvm/Support/FormatVariadic.h"
@@ -172,6 +177,121 @@
   }
   return Fixes;
 }
+
+// Returns the identifiers qualified by an unresolved name. \p Loc is the
+// start location of the unresolved name. For the example below, this returns
+// "::X::Y" that is qualified by unresolved name "clangd":
+// clang::clangd::X::Y
+//~
+llvm::Optional qualifiedByUnresolved(const SourceManager ,
+  SourceLocation Loc,
+  const LangOptions ) {
+  std::string Result;
+
+  SourceLocation NextLoc = Loc;
+  while (auto CCTok = Lexer::findNextToken(NextLoc, SM, LangOpts)) {
+if (!CCTok->is(tok::coloncolon))
+  break;
+auto IDTok = Lexer::findNextToken(CCTok->getLocation(), SM, LangOpts);
+if (!IDTok || !IDTok->is(tok::raw_identifier))
+  break;
+Result.append(("::" + IDTok->getRawIdentifier()).str());
+NextLoc = IDTok->getLocation();
+  }
+  if (Result.empty())
+return llvm::None;
+  return Result;
+}
+
+// An unresolved name and its scope information that can be extracted cheaply.
+struct CheapUnresolvedName {
+  std::string Name;
+  // This is the part of what was typed that was resolved, and it's in its
+  // resolved form not its typed form (think `namespace clang { clangd::x }` -->
+  // `clang::clangd::`).
+  llvm::Optional ResolvedScope;
+
+  // Unresolved part of the scope. When the unresolved name is a specifier, we
+  // use the name that comes after it as the alternative name to resolve and use
+  // the specifier as the extra scope in the accessible scopes.
+  llvm::Optional UnresolvedScope;
+};
+
+// Extracts unresolved name and scope information around \p Unresolved.
+// FIXME: try to merge this with the scope-wrangling code in CodeComplete.
+llvm::Optional extractUnresolvedNameCheaply(
+const SourceManager , const DeclarationNameInfo ,
+CXXScopeSpec *SS, const LangOptions , bool UnresolvedIsSpecifier) {
+  bool Invalid = false;
+  llvm::StringRef Code = SM.getBufferData(
+  SM.getDecomposedLoc(Unresolved.getBeginLoc()).first, );
+  if (Invalid)
+return llvm::None;
+  CheapUnresolvedName Result;
+  Result.Name = Unresolved.getAsString();
+  if (SS && SS->isNotEmpty()) { // "::" or "ns::"
+if (auto *Nested = SS->getScopeRep()) {
+  if (Nested->getKind() == NestedNameSpecifier::Global)
+Result.ResolvedScope = "";
+  else if (const auto *NS = Nested->getAsNamespace()) {
+auto SpecifiedNS = printNamespaceScope(*NS);
+
+// Check the specifier spelled in the source.
+// If the resolved scope doesn't end with the spelled scope. The
+// resolved scope can come from a sema typo correction. For example,
+// sema assumes that "clangd::" is a typo of "clang::" and uses
+// "clang::" as the specified scope in:
+// namespace clang { clangd::X; }
+// In this case, we use the "typo" specifier as extra scope instead
+// of using the scope assumed by sema.
+auto B = SM.getFileOffset(SS->getBeginLoc());
+auto E = SM.getFileOffset(SS->getEndLoc());
+std::string Spelling = (Code.substr(B, E - B) + "::").str();
+if (llvm::StringRef(SpecifiedNS).endswith(Spelling))
+  Result.ResolvedScope = SpecifiedNS;
+else
+  Result.UnresolvedScope = Spelling;
+  } else if (const auto *ANS = Nested->getAsNamespaceAlias()) {
+Result.ResolvedScope = printNamespaceScope(*ANS->getNamespace());
+  } else {
+// We don't fix symbols in scopes that are not top-level e.g. class
+// members, as we 

[PATCH] D58185: [clangd] Handle unresolved scope specifier when fixing includes.

[Eric Liu via Phabricator via cfe-commits]

ioeric updated this revision to Diff 187359.
ioeric added a comment.

- minor cleanup


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D58185

Files:
  clangd/IncludeFixer.cpp
  unittests/clangd/DiagnosticsTests.cpp

Index: unittests/clangd/DiagnosticsTests.cpp
===
--- unittests/clangd/DiagnosticsTests.cpp
+++ unittests/clangd/DiagnosticsTests.cpp
@@ -20,6 +20,7 @@
 namespace clangd {
 namespace {
 
+using testing::_;
 using testing::ElementsAre;
 using testing::Field;
 using testing::IsEmpty;
@@ -369,6 +370,8 @@
 $insert[[]]namespace ns {
 void foo() {
   $unqualified1[[X]] x;
+  // No fix if the unresolved type is used as specifier. (ns::)X::Nested will be
+  // considered the unresolved type.
   $unqualified2[[X]]::Nested n;
 }
 }
@@ -391,10 +394,7 @@
   AllOf(Diag(Test.range("unqualified1"), "unknown type name 'X'"),
 WithFix(Fix(Test.range("insert"), "#include \"x.h\"\n",
 "Add include \"x.h\" for symbol ns::X"))),
-  AllOf(Diag(Test.range("unqualified2"),
- "use of undeclared identifier 'X'"),
-WithFix(Fix(Test.range("insert"), "#include \"x.h\"\n",
-"Add include \"x.h\" for symbol ns::X"))),
+  Diag(Test.range("unqualified2"), "use of undeclared identifier 'X'"),
   AllOf(Diag(Test.range("qualified1"),
  "no type named 'X' in namespace 'ns'"),
 WithFix(Fix(Test.range("insert"), "#include \"x.h\"\n",
@@ -487,6 +487,88 @@
   }
 }
 
+TEST(IncludeFixerTest, UnresolvedNameAsSpecifier) {
+  Annotations Test(R"cpp(
+$insert[[]]namespace ns {
+}
+void g() {  ns::$[[scope]]::X_Y();  }
+  )cpp");
+  auto TU = TestTU::withCode(Test.code());
+  auto Index = buildIndexWithSymbol(
+  SymbolWithHeader{"ns::scope::X_Y", "unittest:///x.h", "\"x.h\""});
+  TU.ExternalIndex = Index.get();
+
+  EXPECT_THAT(
+  TU.build().getDiagnostics(),
+  UnorderedElementsAre(AllOf(
+  Diag(Test.range(), "no member named 'scope' in namespace 'ns'"),
+  WithFix(Fix(Test.range("insert"), "#include \"x.h\"\n",
+  "Add include \"x.h\" for symbol ns::scope::X_Y");
+}
+
+TEST(IncludeFixerTest, UnresolvedSpecifierWithSemaCorrection) {
+  Annotations Test(R"cpp(
+$insert[[]]namespace clang {
+void f() {
+  // "clangd::" will be corrected to "clang::" by Sema.
+  $q1[[clangd]]::$x[[X]] x;
+  $q2[[clangd]]::$ns[[ns]]::Y y;
+}
+}
+  )cpp");
+  auto TU = TestTU::withCode(Test.code());
+  auto Index = buildIndexWithSymbol(
+  {SymbolWithHeader{"clang::clangd::X", "unittest:///x.h", "\"x.h\""},
+   SymbolWithHeader{"clang::clangd::ns::Y", "unittest:///y.h", "\"y.h\""}});
+  TU.ExternalIndex = Index.get();
+
+  EXPECT_THAT(
+  TU.build().getDiagnostics(),
+  UnorderedElementsAre(
+  AllOf(
+  Diag(Test.range("q1"), "use of undeclared identifier 'clangd'; "
+ "did you mean 'clang'?"),
+  WithFix(_, // change clangd to clang
+  Fix(Test.range("insert"), "#include \"x.h\"\n",
+  "Add include \"x.h\" for symbol clang::clangd::X"))),
+  AllOf(
+  Diag(Test.range("x"), "no type named 'X' in namespace 'clang'"),
+  WithFix(Fix(Test.range("insert"), "#include \"x.h\"\n",
+  "Add include \"x.h\" for symbol clang::clangd::X"))),
+  AllOf(
+  Diag(Test.range("q2"), "use of undeclared identifier 'clangd'; "
+ "did you mean 'clang'?"),
+  WithFix(
+  _, // change clangd to clangd
+  Fix(Test.range("insert"), "#include \"y.h\"\n",
+  "Add include \"y.h\" for symbol clang::clangd::ns::Y"))),
+  AllOf(Diag(Test.range("ns"),
+ "no member named 'ns' in namespace 'clang'"),
+WithFix(Fix(
+Test.range("insert"), "#include \"y.h\"\n",
+"Add include \"y.h\" for symbol clang::clangd::ns::Y");
+}
+
+TEST(IncludeFixerTest, SpecifiedScopeIsNamespaceAlias) {
+  Annotations Test(R"cpp(
+$insert[[]]namespace a {}
+namespace b = a;
+namespace c {
+  b::$[[X]] x;
+}
+  )cpp");
+  auto TU = TestTU::withCode(Test.code());
+  auto Index = buildIndexWithSymbol(
+  SymbolWithHeader{"a::X", "unittest:///x.h", "\"x.h\""});
+  TU.ExternalIndex = Index.get();
+
+  EXPECT_THAT(TU.build().getDiagnostics(),
+  UnorderedElementsAre(AllOf(
+  Diag(Test.range(), "no type named 'X' in namespace 'a'"),
+  WithFix(Fix(Test.range("insert"), "#include \"x.h\"\n",
+  "Add include \"x.h\" for symbol a::X");
+}
+
 } // namespace
 } // namespace clangd
 } // namespace 

[PATCH] D58185: [clangd] Handle unresolved scope specifier when fixing includes.

[Eric Liu via Phabricator via cfe-commits]

ioeric updated this revision to Diff 187343.
ioeric marked 16 inline comments as done.
ioeric added a comment.

- address review comment


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D58185

Files:
  clangd/IncludeFixer.cpp
  unittests/clangd/DiagnosticsTests.cpp

Index: unittests/clangd/DiagnosticsTests.cpp
===
--- unittests/clangd/DiagnosticsTests.cpp
+++ unittests/clangd/DiagnosticsTests.cpp
@@ -20,6 +20,7 @@
 namespace clangd {
 namespace {
 
+using testing::_;
 using testing::ElementsAre;
 using testing::Field;
 using testing::IsEmpty;
@@ -369,6 +370,8 @@
 $insert[[]]namespace ns {
 void foo() {
   $unqualified1[[X]] x;
+  // No fix if the unresolved type is used as specifier. (ns::)X::Nested will be
+  // considered the unresolved type.
   $unqualified2[[X]]::Nested n;
 }
 }
@@ -391,10 +394,7 @@
   AllOf(Diag(Test.range("unqualified1"), "unknown type name 'X'"),
 WithFix(Fix(Test.range("insert"), "#include \"x.h\"\n",
 "Add include \"x.h\" for symbol ns::X"))),
-  AllOf(Diag(Test.range("unqualified2"),
- "use of undeclared identifier 'X'"),
-WithFix(Fix(Test.range("insert"), "#include \"x.h\"\n",
-"Add include \"x.h\" for symbol ns::X"))),
+  Diag(Test.range("unqualified2"), "use of undeclared identifier 'X'"),
   AllOf(Diag(Test.range("qualified1"),
  "no type named 'X' in namespace 'ns'"),
 WithFix(Fix(Test.range("insert"), "#include \"x.h\"\n",
@@ -487,6 +487,88 @@
   }
 }
 
+TEST(IncludeFixerTest, UnresolvedNameAsSpecifier) {
+  Annotations Test(R"cpp(
+$insert[[]]namespace ns {
+}
+void g() {  ns::$[[scope]]::X_Y();  }
+  )cpp");
+  auto TU = TestTU::withCode(Test.code());
+  auto Index = buildIndexWithSymbol(
+  SymbolWithHeader{"ns::scope::X_Y", "unittest:///x.h", "\"x.h\""});
+  TU.ExternalIndex = Index.get();
+
+  EXPECT_THAT(
+  TU.build().getDiagnostics(),
+  UnorderedElementsAre(AllOf(
+  Diag(Test.range(), "no member named 'scope' in namespace 'ns'"),
+  WithFix(Fix(Test.range("insert"), "#include \"x.h\"\n",
+  "Add include \"x.h\" for symbol ns::scope::X_Y");
+}
+
+TEST(IncludeFixerTest, UnresolvedSpecifierWithSemaCorrection) {
+  Annotations Test(R"cpp(
+$insert[[]]namespace clang {
+void f() {
+  // "clangd::" will be corrected to "clang::" by Sema.
+  $q1[[clangd]]::$x[[X]] x;
+  $q2[[clangd]]::$ns[[ns]]::Y y;
+}
+}
+  )cpp");
+  auto TU = TestTU::withCode(Test.code());
+  auto Index = buildIndexWithSymbol(
+  {SymbolWithHeader{"clang::clangd::X", "unittest:///x.h", "\"x.h\""},
+   SymbolWithHeader{"clang::clangd::ns::Y", "unittest:///y.h", "\"y.h\""}});
+  TU.ExternalIndex = Index.get();
+
+  EXPECT_THAT(
+  TU.build().getDiagnostics(),
+  UnorderedElementsAre(
+  AllOf(
+  Diag(Test.range("q1"), "use of undeclared identifier 'clangd'; "
+ "did you mean 'clang'?"),
+  WithFix(_, // change clangd to clang
+  Fix(Test.range("insert"), "#include \"x.h\"\n",
+  "Add include \"x.h\" for symbol clang::clangd::X"))),
+  AllOf(
+  Diag(Test.range("x"), "no type named 'X' in namespace 'clang'"),
+  WithFix(Fix(Test.range("insert"), "#include \"x.h\"\n",
+  "Add include \"x.h\" for symbol clang::clangd::X"))),
+  AllOf(
+  Diag(Test.range("q2"), "use of undeclared identifier 'clangd'; "
+ "did you mean 'clang'?"),
+  WithFix(
+  _, // change clangd to clangd
+  Fix(Test.range("insert"), "#include \"y.h\"\n",
+  "Add include \"y.h\" for symbol clang::clangd::ns::Y"))),
+  AllOf(Diag(Test.range("ns"),
+ "no member named 'ns' in namespace 'clang'"),
+WithFix(Fix(
+Test.range("insert"), "#include \"y.h\"\n",
+"Add include \"y.h\" for symbol clang::clangd::ns::Y");
+}
+
+TEST(IncludeFixerTest, SpecifiedScopeIsNamespaceAlias) {
+  Annotations Test(R"cpp(
+$insert[[]]namespace a {}
+namespace b = a;
+namespace c {
+  b::$[[X]] x;
+}
+  )cpp");
+  auto TU = TestTU::withCode(Test.code());
+  auto Index = buildIndexWithSymbol(
+  SymbolWithHeader{"a::X", "unittest:///x.h", "\"x.h\""});
+  TU.ExternalIndex = Index.get();
+
+  EXPECT_THAT(TU.build().getDiagnostics(),
+  UnorderedElementsAre(AllOf(
+  Diag(Test.range(), "no type named 'X' in namespace 'a'"),
+  WithFix(Fix(Test.range("insert"), "#include \"x.h\"\n",
+  "Add include \"x.h\" for symbol a::X");
+}
+
 } // 

[PATCH] D58185: [clangd] Handle unresolved scope specifier when fixing includes.

[Eric Liu via Phabricator via cfe-commits]

ioeric added inline comments.



Comment at: clangd/IncludeFixer.cpp:191
+//~~
+llvm::Optional qualifiedByUnresolved(llvm::StringRef Code,
+  size_t Offset) {

sammccall wrote:
> this isn't wrong per se (conservative, bails out e.g. on unicode)
> But is it much harder to call Lexer::findNextToken twice and expect a raw 
> identifier and ::?
Good idea. Lexer is a better option.



Comment at: clangd/IncludeFixer.cpp:251
+// namespace clang { clangd::X; }
+// In this case, we use the "typo" qualifier as extra scope instead
+// of using the scope assumed by sema.

jkorous wrote:
> Does this work with anonymous namespaces?
I'm not sure... how do you use an anonymous namespace as a scope specifier?



Comment at: clangd/IncludeFixer.cpp:256
+std::string Spelling = (Code.substr(B, E - B) + "::").str();
+vlog("Spelling scope: {0}, SpecifiedNS: {1}", Spelling, SpecifiedNS);
+if (llvm::StringRef(SpecifiedNS).endswith(Spelling))

sammccall wrote:
> I don't think this vlog is useful as-is (quite far down the stack with no 
> context)
> Did you intend to remove it?
Indeed. Thanks for the catch!



Comment at: clangd/IncludeFixer.cpp:271
+  if (IsUnrsolvedSpecifier) {
+// If the unresolved name is a qualifier e.g.
+//  clang::clangd::X

jkorous wrote:
> Is the term `qualifier` applicable here? (Honest question.)
> 
> It seems like C++ grammar uses `specifier` (same as you in 
> `IsUnrsolvedSpecifier `)
> http://www.nongnu.org/hcb/#nested-name-specifier
You've raised a good point. We've been using `specifier` and `qualifier` 
interchangeably in the project. But specifier does seem to be a more 
appropriate name to use.  I've fixed uses in this file. Uses in other files 
probably need a separate cleanup.



Comment at: clangd/IncludeFixer.cpp:322
 UnresolvedName Unresolved;
 Unresolved.Name = Typo.getAsString();
 Unresolved.Loc = Typo.getBeginLoc();

sammccall wrote:
> Following up on our offline discussion :-)
> I think that since `extractSpecifiedScopes` can want to modify the name, we 
> should just expand that function's signature/responsibility to always 
> determine the name.
> 
> So we'd pass the `const DeclarationNameInfo&` to `extractSpecifiedScopes`, 
> and it would return a struct `{optional ResolvedScope; 
> optional UnresolvedScope; string Name}`. 
> Maybe need to call them `CheapUnresolvedName`/`extractUnresolvedNameCheaply` 
> or  similar.
> But I think the code change is small.
Sounds good. Thanks!


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D58185



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