[PATCH] D50375: [clangd] Share getSymbolID implementation.

[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/AST.cpp:59
+  llvm::SmallString<128> USR;
+  if (index::generateUSRForDecl(D, USR)) {
+return None;

nit: no braces



Comment at: clangd/AST.h:38
+/// Gets the symbol ID for a declaration.
+/// Returns None if fails.
+llvm::Optional getSymbolID(const Decl *D);

nit: this isn't necessary a failure. `D` might not have USR. Maybe `..., if 
possible.` like the original wording?



Comment at: clangd/CodeComplete.cpp:399
   case CodeCompletionResult::RK_Pattern: {
-llvm::SmallString<128> USR;
-if (/*Ignore=*/clang::index::generateUSRForDecl(R.Declaration, USR))
-  return None;
-return SymbolID(USR);
+return clang::clangd::getSymbolID(R.Declaration);
   }

No need for namespace qualifiers?


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D50375



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

[PATCH] D50385: [clangd] Collect symbol occurrences from AST.

[Eric Liu via Phabricator via cfe-commits]

ioeric added a comment.

2 high-level questions:

1. What's the reason for having a separate `SymbolOccurrenceSlab`? Could store 
occurrences as extra payload of `Symbol`?

2. Could we merge `SymbolOccurrenceCollector` into the existing 
`SymbolCollector`? They look a lot alike. Having another index data consumer 
seems like more overhead on the user side.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D50385



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

[PATCH] D50446: [clangd] Record the currently active file in context for codeCompletion and findDefinitions.

[Eric Liu via Phabricator via cfe-commits]

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

This allows implementations like different symbol indexes to know what
the current active file is. For example, some customized index implementation
might decide to only return results for some files.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D50446

Files:
  clangd/ClangdServer.cpp
  clangd/Context.cpp
  clangd/Context.h


Index: clangd/Context.h
===
--- clangd/Context.h
+++ clangd/Context.h
@@ -217,6 +217,11 @@
   WithContext Restore;
 };
 
+// The active file in a clangd request (e.g. file in which code completion or
+// go-to-definition is triggered). This is not always set and has to be set in
+// individual requests.
+extern const clang::clangd::Key kActiveFile;
+
 } // namespace clangd
 } // namespace clang
 
Index: clangd/Context.cpp
===
--- clangd/Context.cpp
+++ clangd/Context.cpp
@@ -32,5 +32,7 @@
   return Replacement;
 }
 
+const clang::clangd::Key kActiveFile;
+
 } // namespace clangd
 } // namespace clang
Index: clangd/ClangdServer.cpp
===
--- clangd/ClangdServer.cpp
+++ clangd/ClangdServer.cpp
@@ -159,6 +159,7 @@
 
 auto PreambleData = IP->Preamble;
 
+WithContextValue WithFileName(kActiveFile, File);
 // FIXME(ibiryukov): even if Preamble is non-null, we may want to check
 // both the old and the new version in case only one of them matches.
 CodeCompleteResult Result = clangd::codeComplete(
@@ -297,10 +298,13 @@
 
 void ClangdServer::findDefinitions(PathRef File, Position Pos,
Callback> CB) {
-  auto Action = [Pos, this](Callback> CB,
-llvm::Expected InpAST) {
+  std::string ActiveFile = File;
+  auto Action = [Pos, ActiveFile, this](Callback> CB,
+llvm::Expected InpAST) {
 if (!InpAST)
   return CB(InpAST.takeError());
+
+WithContextValue WithFileName(kActiveFile, ActiveFile);
 CB(clangd::findDefinitions(InpAST->AST, Pos, Index));
   };
 


Index: clangd/Context.h
===
--- clangd/Context.h
+++ clangd/Context.h
@@ -217,6 +217,11 @@
   WithContext Restore;
 };
 
+// The active file in a clangd request (e.g. file in which code completion or
+// go-to-definition is triggered). This is not always set and has to be set in
+// individual requests.
+extern const clang::clangd::Key kActiveFile;
+
 } // namespace clangd
 } // namespace clang
 
Index: clangd/Context.cpp
===
--- clangd/Context.cpp
+++ clangd/Context.cpp
@@ -32,5 +32,7 @@
   return Replacement;
 }
 
+const clang::clangd::Key kActiveFile;
+
 } // namespace clangd
 } // namespace clang
Index: clangd/ClangdServer.cpp
===
--- clangd/ClangdServer.cpp
+++ clangd/ClangdServer.cpp
@@ -159,6 +159,7 @@
 
 auto PreambleData = IP->Preamble;
 
+WithContextValue WithFileName(kActiveFile, File);
 // FIXME(ibiryukov): even if Preamble is non-null, we may want to check
 // both the old and the new version in case only one of them matches.
 CodeCompleteResult Result = clangd::codeComplete(
@@ -297,10 +298,13 @@
 
 void ClangdServer::findDefinitions(PathRef File, Position Pos,
Callback> CB) {
-  auto Action = [Pos, this](Callback> CB,
-llvm::Expected InpAST) {
+  std::string ActiveFile = File;
+  auto Action = [Pos, ActiveFile, this](Callback> CB,
+llvm::Expected InpAST) {
 if (!InpAST)
   return CB(InpAST.takeError());
+
+WithContextValue WithFileName(kActiveFile, ActiveFile);
 CB(clangd::findDefinitions(InpAST->AST, Pos, Index));
   };
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D50337: [clangd] DexIndex implementation prototype

[Eric Liu via Phabricator via cfe-commits]

ioeric added inline comments.



Comment at: clang-tools-extra/clangd/index/MemIndex.h:45
   // Index is a set of symbols that are deduplicated by symbol IDs.
-  // FIXME: build smarter index structure.
   llvm::DenseMap Index;

I think this FIXME still applies here. This can probably go away when we 
completely get rid of MemIndex.



Comment at: clang-tools-extra/clangd/index/dex/DexIndex.cpp:31
+
+// Symbols are sorted by symbol qualities so that items in the posting 
lists
+// are stored in the descending order of symbol quality.

Couldn we build the inverted index also outside of the critical section? As 
this blocks ongoing index requests, we should do as little work as possible in 
the CS.



Comment at: clang-tools-extra/clangd/index/dex/DexIndex.cpp:40
+for (DocID SymbolRank = 0; SymbolRank < Symbols->size(); ++SymbolRank) {
+  const auto Sym = (*Symbols)[SymbolRank];
+  for (const auto &Token : generateSearchTokens(*Sym)) {

nit: `const auto * Sym`



Comment at: clang-tools-extra/clangd/index/dex/DexIndex.cpp:57
+  FuzzyMatcher Filter(Req.Query);
+  bool More;
+  {

nit: Initialize to false



Comment at: clang-tools-extra/clangd/index/dex/DexIndex.cpp:67
+// also implemented in that approach.
+// FIXME(kbobyrev): Code sharing is not very pleasant here, is it better to
+// dispatch long and short queries to internal helper functions?

I'm not quite sure what this FIXME means. What code do you want to share 
between them? 

But we do want to refactor the code a bit to make it easier to follow.



Comment at: clang-tools-extra/clangd/index/dex/DexIndex.cpp:92
+  // otherwise the iterator won't have any children and will be invalid.
+  if (ScopeIterators.size())
+TopLevelChildren.push_back(createOr(move(ScopeIterators)));

Can we avoid creating scope iterators in the first place if they are not going 
to be used?



Comment at: clang-tools-extra/clangd/index/dex/DexIndex.cpp:98
+  // Req.MaxCandidateCount. That would require implementing Limit iterator.
+  SymbolDocIDs = consume(*QueryIterator);
+  More = SymbolDocIDs.size() > Req.MaxCandidateCount;

It seems that the lack of limiting iterators can make the query very 
inefficient. Maybe we should implement the limiting iterators before getting 
the index in, in case people start using it before it's ready?



Comment at: clang-tools-extra/clangd/index/dex/DexIndex.cpp:102
+  // FIXME(kbobyrev): Don't score items twice.
+  for (const auto &Pair : Index) {
+const auto Sym = Pair.second;

Could you document what the approach you are taking to handle short queries? It 
seems that this can be very expensive, for example, if all matching symbols are 
at the end of the DenseMap. Short queries happen very often, so we need to make 
sure index handles them efficiently.



Comment at: clang-tools-extra/clangd/index/dex/DexIndex.cpp:102
+  // FIXME(kbobyrev): Don't score items twice.
+  for (const auto &Pair : Index) {
+const auto Sym = Pair.second;

ioeric wrote:
> Could you document what the approach you are taking to handle short queries? 
> It seems that this can be very expensive, for example, if all matching 
> symbols are at the end of the DenseMap. Short queries happen very often, so 
> we need to make sure index handles them efficiently.
Did you mean to iterate on `Symbols` which is sorted by quality?



Comment at: clang-tools-extra/clangd/index/dex/DexIndex.h:8
+//
+//===--===//
+

I think this file could use some high-level documentation.



Comment at: clang-tools-extra/clangd/index/dex/DexIndex.h:27
+public:
+  void build(std::shared_ptr> Symbols);
+

There is some assumption about `Symbols` (like`MemIndex::build`). Please add 
documentation.



Comment at: clang-tools-extra/clangd/index/dex/DexIndex.h:31
+  // out to Index.h?
+  static std::unique_ptr build(SymbolSlab Slab);
+

Do we need this for this patch? If not, I'd suggest leaving it out and revisit 
when we actually need it (e.g. when replacing MemIndex); otherwise, I think 
what we want is basically a function that takes a `SymbolSlab` and returns 
`std::shared_ptr>`. This can probably live in 
Index.h as you suggested.



Comment at: clang-tools-extra/clangd/index/dex/DexIndex.h:47
+  std::shared_ptr> Symbols;
+  llvm::DenseMap Index;
+  mutable std::mutex Mutex;

Unlike `MemIndex` where this is used as an actual index, here it's simply a 
lookup table, IIUC? Maybe just `SymbolsByID` or `lookupTable`?

[PATCH] D50446: [clangd] Record the currently active file in context for codeCompletion and findDefinitions.

[Eric Liu via Phabricator via cfe-commits]

ioeric added a comment.

In https://reviews.llvm.org/D50446#1192282, @ilya-biryukov wrote:

> Short summary of the offline discussion: I suggest adding this parameter into 
> the corresponding requests of the index (FuzzyFindRequest, 
> FindDefinitionsRequest) instead of stashing it in the context. Context has 
> all the same problems as the global variables, so we should try to avoid 
> using it where possible.


Thanks for the summary! Just for the record, my concerns were:

1. The concept of active files are not really index-specific.
2. We need to do this for all 3 different requests (need "scary" comments 3 
places?).
3. The context value seems to be trivial enough and hard to be misused.

> On the other hand, where this key is stored might not be terribly important 
> if we don't have too many usages and remove it when we can workaround the 
> limitations that we're currently facing.




Comment at: clangd/ClangdServer.cpp:162
 
+WithContextValue WithFileName(kActiveFile, File);
 // FIXME(ibiryukov): even if Preamble is non-null, we may want to check

ilya-biryukov wrote:
> If we want to set this value more consistently, maybe do it in 
> TUScheduler::runWithPreamble/runWithAST? 
> All interesting operations that work on files go through those, so it's 
> easier to certify that the file is set correctly.
I have two concerns about doing this:
1. Setting it in the TUScheduler seems to bury this deeper e.g. into scheduling 
logic. Limiting this in specific callbacks right before it's needed seems to 
make it less accessible.

2. There are operations like `workspaceSymbols` (although not handled in this 
patch)  that do not go through TUScheduler.



Comment at: clangd/Context.cpp:35
 
+const clang::clangd::Key kActiveFile;
+

ilya-biryukov wrote:
> ilya-biryukov wrote:
> > Maybe move it somewhere else? E.g. to `Index.h`, since that's where it's 
> > supposed to be used to workaround current limitations?
> > `Context.h/cpp` is a general-purpose support library for the contexts, 
> > application-specific keys do not fit in well there.
> Maybe use `std::string`? Contexts can easily be cloned/migrated between 
> threads, so it's very easy to end up with pointers to dead strings here.
I intentionally used `StringRef` here to make it harder to use and hoped that 
users would be more careful when using this. For the current simple use cases, 
`StringRef` should be sufficient IMO.



Comment at: clangd/Context.cpp:35
 
+const clang::clangd::Key kActiveFile;
+

ioeric wrote:
> ilya-biryukov wrote:
> > ilya-biryukov wrote:
> > > Maybe move it somewhere else? E.g. to `Index.h`, since that's where it's 
> > > supposed to be used to workaround current limitations?
> > > `Context.h/cpp` is a general-purpose support library for the contexts, 
> > > application-specific keys do not fit in well there.
> > Maybe use `std::string`? Contexts can easily be cloned/migrated between 
> > threads, so it's very easy to end up with pointers to dead strings here.
> I intentionally used `StringRef` here to make it harder to use and hoped that 
> users would be more careful when using this. For the current simple use 
> cases, `StringRef` should be sufficient IMO.
We should probably look for some other place to put this, but I don't think 
`Index.h` is the right place. Although context might not be the best place to 
host the active file, the active file seems to be a well-defined concept in 
clangd in general and not index-specific.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D50446



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

[PATCH] D50446: [clangd] Record the currently active file in context for codeCompletion and findDefinitions.

[Eric Liu via Phabricator via cfe-commits]

ioeric updated this revision to Diff 159746.
ioeric marked 3 inline comments as done.
ioeric added a comment.
Herald added a subscriber: javed.absar.

- Addressed review comments.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D50446

Files:
  clangd/TUScheduler.cpp
  clangd/TUScheduler.h
  unittests/clangd/TUSchedulerTests.cpp

Index: unittests/clangd/TUSchedulerTests.cpp
===
--- unittests/clangd/TUSchedulerTests.cpp
+++ unittests/clangd/TUSchedulerTests.cpp
@@ -197,20 +197,22 @@
 {
   WithContextValue WithNonce(NonceKey, ++Nonce);
   S.update(File, Inputs, WantDiagnostics::Auto,
-   [Nonce, &Mut,
+   [File, Nonce, &Mut,
 &TotalUpdates](llvm::Optional> Diags) {
  EXPECT_THAT(Context::current().get(NonceKey),
  Pointee(Nonce));
 
  std::lock_guard Lock(Mut);
  ++TotalUpdates;
+ EXPECT_EQ(File,
+   *TUScheduler::getFileBeingProcessedInContext());
});
 }
 
 {
   WithContextValue WithNonce(NonceKey, ++Nonce);
   S.runWithAST("CheckAST", File,
-   [Inputs, Nonce, &Mut,
+   [File, Inputs, Nonce, &Mut,
 &TotalASTReads](llvm::Expected AST) {
  EXPECT_THAT(Context::current().get(NonceKey),
  Pointee(Nonce));
@@ -221,23 +223,27 @@
 
  std::lock_guard Lock(Mut);
  ++TotalASTReads;
+ EXPECT_EQ(
+ File,
+ *TUScheduler::getFileBeingProcessedInContext());
});
 }
 
 {
   WithContextValue WithNonce(NonceKey, ++Nonce);
-  S.runWithPreamble("CheckPreamble", File,
-[Inputs, Nonce, &Mut, &TotalPreambleReads](
-llvm::Expected Preamble) {
-  EXPECT_THAT(Context::current().get(NonceKey),
-  Pointee(Nonce));
-
-  ASSERT_TRUE((bool)Preamble);
-  EXPECT_EQ(Preamble->Contents, Inputs.Contents);
-
-  std::lock_guard Lock(Mut);
-  ++TotalPreambleReads;
-});
+  S.runWithPreamble(
+  "CheckPreamble", File,
+  [File, Inputs, Nonce, &Mut, &TotalPreambleReads](
+  llvm::Expected Preamble) {
+EXPECT_THAT(Context::current().get(NonceKey), Pointee(Nonce));
+
+ASSERT_TRUE((bool)Preamble);
+EXPECT_EQ(Preamble->Contents, Inputs.Contents);
+
+std::lock_guard Lock(Mut);
+++TotalPreambleReads;
+EXPECT_EQ(File, *TUScheduler::getFileBeingProcessedInContext());
+  });
 }
   }
 }
Index: clangd/TUScheduler.h
===
--- clangd/TUScheduler.h
+++ clangd/TUScheduler.h
@@ -122,6 +122,13 @@
   /// an LRU cache.
   class ASTCache;
 
+  // The file being built/processed in the current thread. This is a hack in
+  // order to get the file name into the index implementations. Do not depend on
+  // this inside clangd.
+  // FIXME: remove this when there is proper index support via build system
+  // integration.
+  static llvm::Optional getFileBeingProcessedInContext();
+
 private:
   const bool StorePreamblesInMemory;
   const std::shared_ptr PCHOps;
@@ -135,6 +142,7 @@
   llvm::Optional WorkerThreads;
   std::chrono::steady_clock::duration UpdateDebounce;
 };
+
 } // namespace clangd
 } // namespace clang
 
Index: clangd/TUScheduler.cpp
===
--- clangd/TUScheduler.cpp
+++ clangd/TUScheduler.cpp
@@ -63,6 +63,14 @@
 class ASTWorker;
 }
 
+static const clang::clangd::Key kFileBeingProcessed;
+
+llvm::Optional TUScheduler::getFileBeingProcessedInContext() {
+  if (auto *File = Context::current().get(kFileBeingProcessed))
+return StringRef(*File);
+  return llvm::None;
+}
+
 /// An LRU cache of idle ASTs.
 /// Because we want to limit the overall number of these we retain, the cache
 /// owns ASTs (and may evict them) while their workers are idle.
@@ -491,8 +499,9 @@
   {
 std::lock_guard Lock(Mutex);
 assert(!Done && "running a task after stop()");
-Requests.push_back({std::move(Task), Name, steady_clock::now(),
-Context::current().clone(), UpdateType});
+Requests.push_back(
+{std::move(Task), Name, steady_clock::now(),
+ Context::current().derive(kFileBeingProcessed, FileName), UpdateType});
   }
   Request

[PATCH] D50446: [clangd] Record the currently active file in context for codeCompletion and findDefinitions.

[Eric Liu via Phabricator via cfe-commits]

ioeric added a comment.

Ok, I am convinced :) Putting the context key into TUScheduler.cpp and exposing 
a static method to access it sound like a better idea afterall. Thanks for the 
suggestions!


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D50446



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

[PATCH] D50446: [clangd] Record the file being processed in a TUScheduler thread in context.

[Eric Liu via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL339320: [clangd] Record the file being processed in a 
TUScheduler thread in context. (authored by ioeric, committed by ).
Herald added a subscriber: llvm-commits.

Repository:
  rL LLVM

https://reviews.llvm.org/D50446

Files:
  clang-tools-extra/trunk/clangd/TUScheduler.cpp
  clang-tools-extra/trunk/clangd/TUScheduler.h
  clang-tools-extra/trunk/unittests/clangd/TUSchedulerTests.cpp

Index: clang-tools-extra/trunk/clangd/TUScheduler.cpp
===
--- clang-tools-extra/trunk/clangd/TUScheduler.cpp
+++ clang-tools-extra/trunk/clangd/TUScheduler.cpp
@@ -63,6 +63,14 @@
 class ASTWorker;
 }
 
+static const clang::clangd::Key kFileBeingProcessed;
+
+llvm::Optional TUScheduler::getFileBeingProcessedInContext() {
+  if (auto *File = Context::current().get(kFileBeingProcessed))
+return StringRef(*File);
+  return llvm::None;
+}
+
 /// An LRU cache of idle ASTs.
 /// Because we want to limit the overall number of these we retain, the cache
 /// owns ASTs (and may evict them) while their workers are idle.
@@ -491,8 +499,9 @@
   {
 std::lock_guard Lock(Mutex);
 assert(!Done && "running a task after stop()");
-Requests.push_back({std::move(Task), Name, steady_clock::now(),
-Context::current().clone(), UpdateType});
+Requests.push_back(
+{std::move(Task), Name, steady_clock::now(),
+ Context::current().derive(kFileBeingProcessed, FileName), UpdateType});
   }
   RequestsCV.notify_all();
 }
@@ -734,10 +743,12 @@
 Action(InputsAndPreamble{Contents, Command, Preamble.get()});
   };
 
-  PreambleTasks->runAsync("task:" + llvm::sys::path::filename(File),
-  Bind(Task, std::string(Name), std::string(File),
-   It->second->Contents, It->second->Command,
-   Context::current().clone(), std::move(Action)));
+  PreambleTasks->runAsync(
+  "task:" + llvm::sys::path::filename(File),
+  Bind(Task, std::string(Name), std::string(File), It->second->Contents,
+   It->second->Command,
+   Context::current().derive(kFileBeingProcessed, File),
+   std::move(Action)));
 }
 
 std::vector>
Index: clang-tools-extra/trunk/clangd/TUScheduler.h
===
--- clang-tools-extra/trunk/clangd/TUScheduler.h
+++ clang-tools-extra/trunk/clangd/TUScheduler.h
@@ -122,6 +122,13 @@
   /// an LRU cache.
   class ASTCache;
 
+  // The file being built/processed in the current thread. This is a hack in
+  // order to get the file name into the index implementations. Do not depend on
+  // this inside clangd.
+  // FIXME: remove this when there is proper index support via build system
+  // integration.
+  static llvm::Optional getFileBeingProcessedInContext();
+
 private:
   const bool StorePreamblesInMemory;
   const std::shared_ptr PCHOps;
@@ -135,6 +142,7 @@
   llvm::Optional WorkerThreads;
   std::chrono::steady_clock::duration UpdateDebounce;
 };
+
 } // namespace clangd
 } // namespace clang
 
Index: clang-tools-extra/trunk/unittests/clangd/TUSchedulerTests.cpp
===
--- clang-tools-extra/trunk/unittests/clangd/TUSchedulerTests.cpp
+++ clang-tools-extra/trunk/unittests/clangd/TUSchedulerTests.cpp
@@ -197,20 +197,22 @@
 {
   WithContextValue WithNonce(NonceKey, ++Nonce);
   S.update(File, Inputs, WantDiagnostics::Auto,
-   [Nonce, &Mut,
+   [File, Nonce, &Mut,
 &TotalUpdates](llvm::Optional> Diags) {
  EXPECT_THAT(Context::current().get(NonceKey),
  Pointee(Nonce));
 
  std::lock_guard Lock(Mut);
  ++TotalUpdates;
+ EXPECT_EQ(File,
+   *TUScheduler::getFileBeingProcessedInContext());
});
 }
 
 {
   WithContextValue WithNonce(NonceKey, ++Nonce);
   S.runWithAST("CheckAST", File,
-   [Inputs, Nonce, &Mut,
+   [File, Inputs, Nonce, &Mut,
 &TotalASTReads](llvm::Expected AST) {
  EXPECT_THAT(Context::current().get(NonceKey),
  Pointee(Nonce));
@@ -221,23 +223,27 @@
 
  std::lock_guard Lock(Mut);
  ++TotalASTReads;
+ EXPECT_EQ(
+ File,
+ *TUScheduler::getFileBeingProcessedInContext());
});
 }
 
 {
   WithContextValue WithNonce(NonceKey, ++Nonce);
-  S.runWithPreamble("CheckPreamble", File,
-[Inputs, Nonce, &Mut, &TotalPreambleR

[PATCH] D50385: [clangd] Collect symbol occurrences from AST.

[Eric Liu via Phabricator via cfe-commits]

ioeric added a comment.

In https://reviews.llvm.org/D50385#1193545, @hokein wrote:

> In https://reviews.llvm.org/D50385#1191914, @ioeric wrote:
>
> > 2 high-level questions:
> >
> > 1. What's the reason for having a separate `SymbolOccurrenceSlab`? Could 
> > store occurrences as extra payload of `Symbol`?
>
>
> Storing occurrences in `Symbol` structure is easy to misuse by users IMO -- 
> if we go through this way, we will end up having a `getOccurrences`-like 
> method in `Symbol` structure. Once users get the `Symbol` instance, it is 
> natural for them to call `getOccurrences` to get all occurrences of the 
> symbol. However this `getOccurrences` method doesn't do what users expected 
> (just returning an incomplete set of results or empty). To query the symbol 
> occurrences, we should always use index interface.
>
> Therefore, I think we should try to avoid these confusions in the design.


Hmm, I think this is the same for other symbol payload e.g. definition can be 
missing for a symbol. And it seems to me that the concern is on the SymbolSlab 
level: if a slab is for a single TU, users should expect missing information; 
if a slab is merged from all TUs, then users can expect "complete" information. 
I think it's reasonable to assume that users of SymbolSlab are aware of this. I 
think it's probably not worth the overhead of maintaining and using two 
separate slabs.

>> 2. Could we merge `SymbolOccurrenceCollector` into the existing 
>> `SymbolCollector`? They look a lot alike. Having another index data consumer 
>> seems like more overhead on the user side.
> 
> The `SymbolOccurrenceCollector` has many responsibilities (collecting 
> declaration, definition, code completion information etc), and the code is 
> growing complex now. Merging the `SymbolOccurrenceCollector` to it will make 
> it more

Although the existing `SymbolCollector` supports different options, I think it 
still has a pretty well-defined responsibility: gather information about 
symbols. IMO, cross-reference is one of the property of symbol, and I don't see 
strong reasons to keep them separated.

> complicated -- we will introduce more option flags like 
> `collect-symbol-only`, `collect-occurrence-only` to configure it for our 
> different use cases (we need to the implementation detail clearly in order to 
> make a correct option for `SymbolCollector`).

I think these options are reasonable if they turn out to be necessary. And 
making the SymbolCollector more complicated doesn't seem to be a problem if we 
are indeed doing more complicated work, but I don't think this would turn into 
a big problem as logic of xrefs seems pretty isolated.  Conversely, I think 
implementing xrefs in a separate class would likely to cause more duplicate and 
maintenance, e.g. two sets of options, two sets of initializations or life-time 
tracking of collectors (they look a lot alike), the same boilerplate factory 
code in tests, passing around two collectors in user code.

> And I can foresee these two collectors might be run at different point 
> (runWithPreamble vs runWithAST) in dynamic index.

With some options, this should be a problem I think?


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D50385



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

[PATCH] D50500: [clangd] Allow consuming limited number of items

[Eric Liu via Phabricator via cfe-commits]

ioeric added a comment.

Could you add test? ;)




Comment at: clang-tools-extra/clangd/index/dex/Iterator.cpp:223
   std::vector Result;
-  for (; !It.reachedEnd(); It.advance())
+  for (size_t Retreived = 0; !It.reachedEnd() && Retreived < Limit;
+   It.advance())

Where do we increment `Retrieved`?



Comment at: clang-tools-extra/clangd/index/dex/Iterator.cpp:223
   std::vector Result;
-  for (; !It.reachedEnd(); It.advance())
+  for (size_t Retreived = 0; !It.reachedEnd() && Retreived < Limit;
+   It.advance())

ioeric wrote:
> Where do we increment `Retrieved`?
nit: `s/Retreived/Retrieved/`



Comment at: clang-tools-extra/clangd/index/dex/Iterator.h:108
+/// for the number of elements obtained before the iterator is exhausted.
+std::vector consume(Iterator &It,
+   size_t Limit = std::numeric_limits::max());

nit: I think `Size of the returned ...` is just repeating the first two 
sentences. Maybe drop?


https://reviews.llvm.org/D50500



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

[PATCH] D50500: [clangd] Allow consuming limited number of items

[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-tools-extra/unittests/clangd/DexIndexTests.cpp:252
+  auto DocIterator = create(L0);
+  EXPECT_THAT(consume(*DocIterator, 42), ElementsAre(4, 7, 8, 20, 42, 100));
+

nit: maybe use the default parameter here?



Comment at: clang-tools-extra/unittests/clangd/DexIndexTests.cpp:260
+
+  auto Root = createAnd(createAnd(create(L1), create(L2)),
+createOr(create(L3), create(L4), create(L5)));

The AND iterator case doesn't seem to add more coverage. Maybe drop?


https://reviews.llvm.org/D50500



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

[PATCH] D50517: [clangd] Generate incomplete trigrams for the Dex index

[Eric Liu via Phabricator via cfe-commits]

ioeric added a comment.

Thanks for the patch!

In https://reviews.llvm.org/D50517#1194955, @kbobyrev wrote:

> Complete the tests, finish the implementation.
>
> One thought about prefix match suggestion: we should either make it more 
> explicit for the index (e.g. introduce `prefixMatch` and dispatch 
> `fuzzyMatch` to prefix matching in case query only contains one "true" 
> symbol) or document this properly. While, as I wrote earlier, I totally 
> support the idea of prefix matching queries of length 1 it might not align 
> with some user expectations and it's also very implicit if we just generate 
> tokens this way and don't mention it anywhere in the `DexIndex` 
> implementation.
>
> @ioeric, @ilya-biryukov any thoughts?


(copied my inline comment :)
We should definitely add documentation about it. It should be pretty simple 
IMO. As the behavior should be easy to infer from samples, and it shouldn't be 
too surprising for users, I think it would be OK to consider it as 
implementation detail (like details in how exactly trigrams are generated) 
without exposing new interfaces for them.




Comment at: clang-tools-extra/clangd/index/dex/Trigram.cpp:26
 
-// FIXME(kbobyrev): Deal with short symbol symbol names. A viable approach 
would
-// be generating unigrams and bigrams here, too. This would prevent symbol 
index
-// from applying fuzzy matching on a tremendous number of symbols and allow
-// supplementary retrieval for short queries.
-//
-// Short names (total segment length <3 characters) are currently ignored.
+// FIXME(kbobyrev): Posting lists for incomplete trigrams (one/two symbols) are
+// likely to be very dense and hence require special attention so that the 
index

It's a nice to optimization have when we run into oversized posting lists, but 
this is not necessarily restricted to unigram posting lists. I think the FIXME 
should live near the general posting list code. I think it's probably also ok 
to leave it out; it's hard to forget if we do run into problem in the future ;)



Comment at: clang-tools-extra/clangd/index/dex/Trigram.cpp:74
+// symbol of the identifier.
+if (!FoundFirstSymbol) {
+  FoundFirstSymbol = true;

Could this be pulled out of the loop? I think what we want is just 
`LowercaseIdentifier[0]` right?

I'd probably also pulled that into a function, as the function body is getting 
larger.



Comment at: clang-tools-extra/clangd/index/dex/Trigram.cpp:87
+
+  Chars = {{LowercaseIdentifier[I], LowercaseIdentifier[J], END_MARKER, 
0}};
+  const auto Bigram = Token(Token::Kind::Trigram, Chars.data());

I think we could be more restrictive on bigram generation. I think a bigram 
prefix of identifier and a bigram prefix of the HEAD substring should work 
pretty well in practice. For example, for `StringStartsWith`, you would have 
`st$` and `ss$` (prefix of "SSW").

WDYT?



Comment at: clang-tools-extra/clangd/index/dex/Trigram.cpp:115
+// FIXME(kbobyrev): Correctly handle empty trigrams "$$$".
 std::vector generateQueryTrigrams(llvm::StringRef Query) {
   // Apply fuzzy matching text segmentation.

It seems to me that what we need for short queries is simply:
```
if (Query.empty()) {
   // return empty token
}
if (Query.size() == 1) return {Query + "$$"};
if (Query.size() == 2) return {Query + "$"};

// Longer queries...
```
?



Comment at: clang-tools-extra/clangd/index/dex/Trigram.h:39
+// member of Token even though it's Trigram-specific?
+const auto END_MARKER = '$';
+

Any reason why this should be exposed?



Comment at: clang-tools-extra/clangd/index/dex/Trigram.h:62
 /// belongs to more than one class it is only inserted once.
+// FIXME(kbobyrev): Document somewhere in DexIndex that for queries of size 1
+// it will do prefix matching instead of fuzzy matching on the identifier 
names,

The behavior should be easy to infer from samples. As long as it's not totally 
expected, I think it would be OK to consider treat as implementation detail 
(like details in how trigrams are generated).



Comment at: clang-tools-extra/clangd/index/dex/Trigram.h:74
+/// For short queries (Query contains less than 3 letters and digits) this
+/// returns a single trigram with all valid symbols.
 std::vector generateQueryTrigrams(llvm::StringRef Query);

I'm not quite sure what this means. Could you elaborate?


https://reviews.llvm.org/D50517



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

[PATCH] D50517: [clangd] Generate incomplete trigrams for the Dex index

[Eric Liu via Phabricator via cfe-commits]

ioeric added a comment.

In https://reviews.llvm.org/D50517#1194976, @kbobyrev wrote:

> As discussed offline with @ilya-biryukov, the better approach would be to 
> prefix match first symbols of each distinct identifier piece instead of 
> prefix matching (just looking at the first letter of the identifier) the 
> whole identifier.
>
> Example:
>
> - Query: `"u"`
> - Symbols: `"unique_ptr"`, `"user"`, `"super_user"`
>
>   Current implementation would match `"unique_ptr"` and `"user"` only. 
> Proposed implementation would match all three symbols, because the second 
> piece of `"super_user"` starts with `u`.


I agree that this can be useful sometime, but I suspect it's relatively rare 
and might actually compromise ranking quality for the more common use case e.g. 
the first character users type is the first character of the expected 
identifier.


https://reviews.llvm.org/D50517



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

[PATCH] D50517: [clangd] Generate incomplete trigrams for the Dex index

[Eric Liu via Phabricator via cfe-commits]

ioeric added a comment.

In https://reviews.llvm.org/D50517#1194976, @kbobyrev wrote:

> As discussed offline with @ilya-biryukov, the better approach would be to 
> prefix match first symbols of each distinct identifier piece instead of 
> prefix matching (just looking at the first letter of the identifier) the 
> whole identifier.
>
> Example:
>
> - Query: `"u"`
> - Symbols: `"unique_ptr"`, `"user"`, `"super_user"`
>
>   Current implementation would match `"unique_ptr"` and `"user"` only. 
> Proposed implementation would match all three symbols, because the second 
> piece of `"super_user"` starts with `u`.


And in the case where users want to match `super_user`, I think it's reasonable 
to have users type two more characters and match it with `use`.


https://reviews.llvm.org/D50517



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

[PATCH] D50337: [clangd] DexIndex implementation prototype

[Eric Liu via Phabricator via cfe-commits]

ioeric added inline comments.



Comment at: clang-tools-extra/clangd/index/dex/DexIndex.cpp:29
+  // might be more efficient.
+  std::sort(begin(*Syms), end(*Syms), [](const Symbol *LHS, const Symbol *RHS) 
{
+return quality(*LHS) > quality(*RHS);

Calculating `quality` on each comparison can also get expensive. I think we 
could store the quality.



Comment at: clang-tools-extra/clangd/index/dex/DexIndex.cpp:37
+for (const auto &Token : generateSearchTokens(*Sym)) {
+  if (!TempInvertedIndex.count(Token))
+TempInvertedIndex[Token] = {SymbolRank};

nit: use `try_emplace` to save one lookup? 



Comment at: clang-tools-extra/clangd/index/dex/DexIndex.cpp:62
+  {
+std::lock_guard Lock(Mutex);
+More = Req.Query.size() >= 3 ? fuzzyFindLongQuery(Callback, Req)

I think we should let the helpers grab the lock. Some preparatory work doesn't 
require lock.

FWIW, I think the separation of short and long code paths is heading in a wrong 
direction. And it's also pretty hard to find a very clean abstraction here. For 
example, there is some overlaps in both functions, and `useCallback` seems a 
bit awkward. 

As all this would probably go away after D50517 anyway, I think we could try to 
get that patch landed and incorporate it into this patch? If you prefer to get 
this patch in first, please add `FIXME` somewhere to make it clear that the 
divergence is temporary.



Comment at: clang-tools-extra/clangd/index/dex/DexIndex.cpp:82
+const auto *Sym = (*Symbols)[ID];
+const auto Score = Filter.match(Sym->Name);
+// Match scopes from the query.

nit: avoid `auto`here as the type of `Score` is not obvious here.



Comment at: clang-tools-extra/clangd/index/dex/DexIndex.cpp:84
+// Match scopes from the query.
+// FIXME(kbobyrev): Use scope filtering before and iterate through the
+// posting list with the result. However, since the query might not contain

Put this `FIXME` on the for-loop level as iterating all symbols is the problem.

And I think the `FIXME` could simply be 
```
FIXME(...): This can be very expensive. We should first filter symbols by 
scopes (with scope iterators).
```

We can leave out the details/options as they are not interesting for most 
readers (as they are mostly concerns about scope filtering). In general, we 
should try to keep comments in documentation brief to keep the code shorter and 
easier to read.



Comment at: clang-tools-extra/clangd/index/dex/DexIndex.cpp:94
+  continue;
+if (Score.hasValue()) {
+  // If the requested number of results is already retrieved but there

nit: `if (Score)`



Comment at: clang-tools-extra/clangd/index/dex/DexIndex.cpp:95
+if (Score.hasValue()) {
+  // If the requested number of results is already retrieved but there
+  // are other symbols which match the criteria, just stop processing

The code here is trivial, so the comment seems redundant. 



Comment at: clang-tools-extra/clangd/index/dex/DexIndex.cpp:102
+  }
+  Scores.push_back(std::make_pair(-*Score * quality(*Sym), Sym));
+}

For clarity, `- (*Score) * quality`. 

Please also comment on why we negate the number here?



Comment at: clang-tools-extra/clangd/index/dex/DexIndex.cpp:175
+  // Sort retrieved symbol by Fuzzy Matching score.
+  std::sort(begin(Scores), end(Scores));
+

Shouldn't `Scores` already be sorted for both short query and long query?



Comment at: clang-tools-extra/clangd/index/dex/DexIndex.h:12
+// over symbol tokens, such as fuzzy matching trigrams, scopes, types, etc. Dex
+// is supposed to be a drop-in replacement of MemIndex in the future: while
+// consuming more memory and having longer build stage due to preprocessing, 
Dex

nit: we don't really need to mention MemIndex here as it's likely to be 
replaced soon, and the comment will outdated then.

It's okay to mention why `Dex` is cool though :)



Comment at: clang-tools-extra/clangd/index/dex/DexIndex.h:60
+private:
+  /// Constructs iterators over tokens extracted from the query and exhausts it
+  /// while applying Callback to each symbol in the order of decreasing quality

nit: The comment about implementation details should go with the 
implementation. Same below.



Comment at: clang-tools-extra/clangd/index/dex/DexIndex.h:65
+  const FuzzyFindRequest &Req) const;
+  /// Handles fuzzy matching requests with Req.Query.size() <= 3. This approach
+  /// currently does not use trigram index and query iterators and will be

`Req.Query.size() < 3`?



Comment at: clang-tools-extra/clangd/index/dex/Token.h:84
 
+

[PATCH] D50517: [clangd] Generate incomplete trigrams for the Dex index

[Eric Liu via Phabricator via cfe-commits]

ioeric added inline comments.



Comment at: clang-tools-extra/clangd/index/dex/Trigram.cpp:74
+// symbol of the identifier.
+if (!FoundFirstSymbol) {
+  FoundFirstSymbol = true;

kbobyrev wrote:
> ioeric wrote:
> > Could this be pulled out of the loop? I think what we want is just 
> > `LowercaseIdentifier[0]` right?
> > 
> > I'd probably also pulled that into a function, as the function body is 
> > getting larger.
> Same as elsewhere, if we have `__builtin_whatever` the it's not actually the 
> first symbol of the lowercase identifier.
I would argue that I would start by typing "_" if I actually want 
`__builtin_whatever`. I'm also not sure if this is the case we should optimize 
for as well; __builtin symbols  are already penalized in code completion 
ranking.



Comment at: clang-tools-extra/clangd/index/dex/Trigram.cpp:115
+// FIXME(kbobyrev): Correctly handle empty trigrams "$$$".
 std::vector generateQueryTrigrams(llvm::StringRef Query) {
   // Apply fuzzy matching text segmentation.

kbobyrev wrote:
> ioeric wrote:
> > It seems to me that what we need for short queries is simply:
> > ```
> > if (Query.empty()) {
> >// return empty token
> > }
> > if (Query.size() == 1) return {Query + "$$"};
> > if (Query.size() == 2) return {Query + "$"};
> > 
> > // Longer queries...
> > ```
> > ?
> That would mean that we expect the query to be "valid", i.e. only consist of 
> letters and digits. My concern is about what happens if we have `"u_"` or 
> something similar (`"_u", "_u_", "$u$"`, etc) - in that case we would 
> actually still have to identify the first valid symbol for the trigram, 
> process the string (trim it, etc) which looks very similar to what 
> FuzzyMatching `calculateRoles` does.
> 
> The current approach is rather straightforward and generic, but I can try to 
> change it if you want. My biggest concern is fighting some corner cases and 
> ensuring that the query is "right" on the user (index) side, which might turn 
> out to be more code and ensuring that the "state" is valid throughout the 
> pipeline.
It's not clear what we would want to match with "*_", except for `u_` in 
`unique_ptr` (maybe).  

Generally, as short queries tend to match many more symbols, I think we should 
try to make them more restrictive and optimize for the most common use case. 


https://reviews.llvm.org/D50517



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

[PATCH] D50517: [clangd] Generate incomplete trigrams for the Dex index

[Eric Liu via Phabricator via cfe-commits]

ioeric added inline comments.



Comment at: clang-tools-extra/clangd/index/dex/Trigram.cpp:33
+
+void insertChars(DenseSet &UniqueTrigrams, std::string Chars) {
+  const auto Trigram = Token(Token::Kind::Trigram, Chars);

This is probably neater as a lambda in `generateIdentifierTrigrams `, e.g.

```
auto add = [&](std::string Chars) {
  trigrams.insert(Token(Token::Kind::Trigram, Chars));
}
...
add("abc"); 
```



Comment at: clang-tools-extra/clangd/index/dex/Trigram.cpp:95
+
+  // Generate trigrams only if the first character is the segment start.
+  // Example: "StringStartsWith" would yield "st$", "ss$" but not "tr$".

Do you mean *bigrams* here?



Comment at: clang-tools-extra/clangd/index/dex/Trigram.cpp:96
+  // Generate trigrams only if the first character is the segment start.
+  // Example: "StringStartsWith" would yield "st$", "ss$" but not "tr$".
+  if (Roles[I] == Head) {

Should we also check `Roles[J] == Head `?

As bigram posting lists would be significantly larger than those of trigrams, I 
would suggest being even more restrictive. For example, for "AppleBananaCat", 
the most common short queries would be "ap" and "ab" (for `AB`).



Comment at: clang-tools-extra/clangd/index/dex/Trigram.cpp:119
 
-// FIXME(kbobyrev): Similarly, to generateIdentifierTrigrams, this ignores 
short
-// inputs (total segment length <3 characters).
+// FIXME(kbobyrev): Correctly handle cases like "u_". In this case, we would
+// like to match "u" only. This might also work better with word bounds for

Couldn't we generate a bigram "u_$" in this case? I think we can assume prefix 
matching in this case, if we generate bigram "u_" for identiifers like "u_*".

> In this case, we would like to match "u" only.
Why? If user types "_", I would expect it to be a meaning filter. 



Comment at: clang-tools-extra/clangd/index/dex/Trigram.cpp:141
+  // sufficient, generate a single incomplete trigram for query.
+  if (ValidSymbolsCount < 3) {
+std::string Symbols = {{END_MARKER, END_MARKER, END_MARKER}};

For queries like `__` or `_x`,  I think we can generate tokens "__$" or `_x$`. 



Comment at: clang-tools-extra/clangd/index/dex/Trigram.h:49
+/// result. Incomplete trigrams contain END_MARKER ('$') at the end. The result
+/// also contains bigrams which are either two subsequent Head symbols or Head
+/// and subsequent Tail. This means that for short queries Dex index would do

nit: the term "symbol" here is confusing. Do you mean "character"?



Comment at: clang-tools-extra/clangd/index/dex/Trigram.h:59
+/// * Bigram with the first two symbols (if availible), same logic applies.
+///
 /// Note: the returned list of trigrams does not have duplicates, if any 
trigram

I think the comment can be simplified a bit:
```
This also generates incomplete trigrams for short query scenarios:
  * Empty trigram: "$$$"
  * Unigram: the first character of the identifier.
  * Bigrams: a 2-char prefix of the identifier and a bigram of the first two 
HEAD characters (if it exists).
```


https://reviews.llvm.org/D50517



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

[PATCH] D50517: [clangd] Generate incomplete trigrams for the Dex index

[Eric Liu via Phabricator via cfe-commits]

ioeric added inline comments.



Comment at: clang-tools-extra/clangd/index/dex/Trigram.cpp:31
+/// use it as the special symbol.
+const auto END_MARKER = '$';
+

nit: s/auto/char/

Maybe just use `static` instead of an anonymous namespace just for this.



Comment at: clang-tools-extra/clangd/index/dex/Trigram.cpp:83
+
+  bool FoundFirstHead = false;
+  // Iterate through valid seqneces of three characters Fuzzy Matcher can

It's probably unclear what `FoundFirstHead` and `FoundSecondHead` are for to 
readers without context (i.e. we are looking first two HEADs). I think it's 
would be cleaner if we handle this out side of the look e.g. record the first 
head in the `Next` initialization loop above.



Comment at: clang-tools-extra/clangd/index/dex/Trigram.cpp:138
   DenseSet UniqueTrigrams;
   std::deque Chars;
 

nit: `Chars` is only used in the else branch?



Comment at: clang-tools-extra/clangd/index/dex/Trigram.cpp:143
+  if (ValidSymbolsCount < 3) {
+std::string Symbols = {{END_MARKER, END_MARKER, END_MARKER}};
+  if (LowercaseQuery.size() > 0)

I think this can be simplified as:
```
std::string Chars = LowercaseQuery.substr(std::min(3, LowercaseQuery.size()));
Chars.append(END_MARKER, 3-Chars.size());
UniqueTrigrams.insert(Token(Token::Kind::Trigram, Chars));
```

also nit: avoid using the term "symbol" here.



Comment at: clang-tools-extra/clangd/index/dex/Trigram.cpp:167
 
-Chars.push_back(LowercaseQuery[I]);
+if (Chars.front() == END_MARKER)
+  auto Trigram = Token(Token::Kind::Trigram,

When would this happen? And why are we reversing the trigram and throwing it 
away?



Comment at: clang-tools-extra/clangd/index/dex/Trigram.h:59
+/// * Bigram with the first two symbols (if availible), same logic applies.
+///
 /// Note: the returned list of trigrams does not have duplicates, if any 
trigram

ioeric wrote:
> I think the comment can be simplified a bit:
> ```
> This also generates incomplete trigrams for short query scenarios:
>   * Empty trigram: "$$$"
>   * Unigram: the first character of the identifier.
>   * Bigrams: a 2-char prefix of the identifier and a bigram of the first two 
> HEAD characters (if it exists).
> ```
nit: and the previous paragraph `Special kind of trigrams ...` is redundant now 
IMO.


https://reviews.llvm.org/D50517



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

[PATCH] D50517: [clangd] Generate incomplete trigrams for the Dex index

[Eric Liu via Phabricator via cfe-commits]

ioeric added inline comments.



Comment at: clang-tools-extra/clangd/index/dex/Trigram.cpp:83
+
+  bool FoundFirstHead = false;
+  // Iterate through valid seqneces of three characters Fuzzy Matcher can

ioeric wrote:
> It's probably unclear what `FoundFirstHead` and `FoundSecondHead` are for to 
> readers without context (i.e. we are looking first two HEADs). I think it's 
> would be cleaner if we handle this out side of the look e.g. record the first 
> head in the `Next` initialization loop above.
Sorry, full of typos here: 

I think it would be cleaner if we handle this outside of the loop e.g. record 
the first head in the `Next` initialization loop above.




https://reviews.llvm.org/D50517



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

[PATCH] D50517: [clangd] Generate incomplete trigrams for the Dex index

[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 for the changes!




Comment at: clang-tools-extra/clangd/index/dex/Trigram.cpp:147
+  if (Chars.size() == 3) {
+const auto Trigram =
+Token(Token::Kind::Trigram, std::string(begin(Chars), end(Chars)));

nit: inline this variable? You don't need to `count` below as `insert` 
duplicates for you already.


https://reviews.llvm.org/D50517



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

[PATCH] D50702: [clangd] NFC: Cleanup clangd help message

[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


https://reviews.llvm.org/D50702



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

[PATCH] D50707: NFC: Enforce good formatting across multiple clang-tools-extra files

[Eric Liu via Phabricator via cfe-commits]

ioeric added a comment.

Fixing header guards and file comments is fine, but I'm not a fan of 
reformatting-only changes in source code as they tends to make `git blame` 
harder.


https://reviews.llvm.org/D50707



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

[PATCH] D50727: [clangd] Fetch documentation from the Index during signature help

[Eric Liu via Phabricator via cfe-commits]

ioeric added inline comments.



Comment at: clangd/CodeComplete.cpp:755
+  });
+  log("SigHelp: requested docs for {0} symbols from the index, got {1} "
+  "symbols with non-empty docs in the response",

drive by: I think this should  be `vlog` or `dlog`.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D50727



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

[PATCH] D50740: [SourceManager] isPointWithin: avoid using isBeforeInTranslationUnit, compare buffer offsets directly for lexical correctness

[Eric Liu via Phabricator via cfe-commits]

ioeric added inline comments.



Comment at: lib/Basic/SourceManager.cpp:2035
+ "Passed invalid source location!");
+  assert(Start.isFileID() && End.isFileID() && Loc.isFileID() &&
+ "Passed non-file source location!");

Why do we disallow locations from macro expansions?



Comment at: lib/Basic/SourceManager.cpp:2049
+return false;
+  std::pair EndLoc = getDecomposedLoc(End);
+  // The point must be in the same file as the end location.

nit: the pattern seems repetitive. Maybe pull out a helper/lambda like: 
`isBeforeInSameFile(L1, L2)`?



Repository:
  rC Clang

https://reviews.llvm.org/D50740



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

[PATCH] D50727: [clangd] Fetch documentation from the Index during signature help

[Eric Liu via Phabricator via cfe-commits]

ioeric added inline comments.



Comment at: clangd/CodeComplete.cpp:755
+  });
+  log("SigHelp: requested docs for {0} symbols from the index, got {1} "
+  "symbols with non-empty docs in the response",

ilya-biryukov wrote:
> ioeric wrote:
> > drive by: I think this should  be `vlog` or `dlog`.
> Code completion also logs the number of results from sema, index, etc. using 
> the `log()` call.
> The added log message looks similar, so trying to be consistent with the rest 
> of the code in this file.
> 
> Maybe we should turn all of them into `vlog` or `dlog`, but I'd rather leave 
> it to a separate patch.
I'm not sure which level code completion log messages should be in, but I think 
we should follow the guidelines in the logger documentation instead of the 
existing uses.

Quote from Logger.h
```
// log() is used for information important to understanding a clangd session.
// e.g. the names of LSP messages sent are logged at this level.
// This level could be enabled in production builds to allow later inspection.

// vlog() is used for details often needed for debugging clangd sessions.
// This level would typically be enabled for clangd developers.
```


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D50727



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

[PATCH] D50727: [clangd] Fetch documentation from the Index during signature help

[Eric Liu via Phabricator via cfe-commits]

ioeric added inline comments.



Comment at: clangd/CodeComplete.cpp:755
+  });
+  log("SigHelp: requested docs for {0} symbols from the index, got {1} "
+  "symbols with non-empty docs in the response",

hokein wrote:
> ioeric wrote:
> > ilya-biryukov wrote:
> > > ioeric wrote:
> > > > drive by: I think this should  be `vlog` or `dlog`.
> > > Code completion also logs the number of results from sema, index, etc. 
> > > using the `log()` call.
> > > The added log message looks similar, so trying to be consistent with the 
> > > rest of the code in this file.
> > > 
> > > Maybe we should turn all of them into `vlog` or `dlog`, but I'd rather 
> > > leave it to a separate patch.
> > I'm not sure which level code completion log messages should be in, but I 
> > think we should follow the guidelines in the logger documentation instead 
> > of the existing uses.
> > 
> > Quote from Logger.h
> > ```
> > // log() is used for information important to understanding a clangd 
> > session.
> > // e.g. the names of LSP messages sent are logged at this level.
> > // This level could be enabled in production builds to allow later 
> > inspection.
> > 
> > // vlog() is used for details often needed for debugging clangd sessions.
> > // This level would typically be enabled for clangd developers.
> > ```
> My recent experience of debugging some weird GotoDef issues tells me that log 
> of index should be on production (since it is a non-trivial part in a clangd 
> session), it would be really helpful to understand what is going on. 
I agree that they are useful for debugging, but they might not be interesting 
to end-users. And I think that's why there is `vlog`. Clangd developers could 
use a different log level with `--log` flag.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D50727



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

[PATCH] D50726: [clangd] Show function documentation in sigHelp

[Eric Liu via Phabricator via cfe-commits]

ioeric added inline comments.



Comment at: clangd/CodeCompletionStrings.cpp:81
+std::string
+getDocComment(const ASTContext &Ctx,
+  const CodeCompleteConsumer::OverloadCandidate &Overload,

The function doesn't seem to carry its weight, and the difference from the 
other overload is a bit subtle. Maybe we could expose `getDeclComent` instead?


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D50726



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

[PATCH] D50727: [clangd] Fetch documentation from the Index during signature help

[Eric Liu via Phabricator via cfe-commits]

ioeric added inline comments.



Comment at: clangd/CodeComplete.cpp:755
+  });
+  log("SigHelp: requested docs for {0} symbols from the index, got {1} "
+  "symbols with non-empty docs in the response",

ilya-biryukov wrote:
> ioeric wrote:
> > hokein wrote:
> > > ioeric wrote:
> > > > ilya-biryukov wrote:
> > > > > ioeric wrote:
> > > > > > drive by: I think this should  be `vlog` or `dlog`.
> > > > > Code completion also logs the number of results from sema, index, 
> > > > > etc. using the `log()` call.
> > > > > The added log message looks similar, so trying to be consistent with 
> > > > > the rest of the code in this file.
> > > > > 
> > > > > Maybe we should turn all of them into `vlog` or `dlog`, but I'd 
> > > > > rather leave it to a separate patch.
> > > > I'm not sure which level code completion log messages should be in, but 
> > > > I think we should follow the guidelines in the logger documentation 
> > > > instead of the existing uses.
> > > > 
> > > > Quote from Logger.h
> > > > ```
> > > > // log() is used for information important to understanding a clangd 
> > > > session.
> > > > // e.g. the names of LSP messages sent are logged at this level.
> > > > // This level could be enabled in production builds to allow later 
> > > > inspection.
> > > > 
> > > > // vlog() is used for details often needed for debugging clangd 
> > > > sessions.
> > > > // This level would typically be enabled for clangd developers.
> > > > ```
> > > My recent experience of debugging some weird GotoDef issues tells me that 
> > > log of index should be on production (since it is a non-trivial part in a 
> > > clangd session), it would be really helpful to understand what is going 
> > > on. 
> > I agree that they are useful for debugging, but they might not be 
> > interesting to end-users. And I think that's why there is `vlog`. Clangd 
> > developers could use a different log level with `--log` flag.
> I agree that `vlog` is probably a better fit here, but still think we should 
> change it across `CodeComplete.cpp` in a single commit, rather than partially 
> apply the guidelines to different parts of the file.
> 
> However, if everyone agrees we should use `vlog` here, happy to use `vlog` 
> too and also send a patch to update all the rest of the usages.
> The situation I'm trying to avoid is:
> 1. We commit `vlog` here.
> 2. Someone argues that using `log` is actually better and we decide to not 
> change other usages to `log`.
> 3. We end up with inconsistent choices across this file: `vlog` for signature 
> help and `log` for code completion.
> 
> But if there's an agreement that we should use `vlog` everywhere, more than 
> happy to change it :-)
That sounds good to me. Thanks!


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D50727



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

[PATCH] D50835: [clangd] Add parantheses while auto-completing functions.

[Eric Liu via Phabricator via cfe-commits]

ioeric added inline comments.



Comment at: clangd/CodeComplete.h:85
+
+  /// Enables cursor to be moved around according to completions needs even 
when
+  /// snippets are disabled. For example selecting a function with parameters 
as

IIRC, the goal of this patch is to allow disabling snippet templates for 
function parameters while still allow appending `()` or `($0)` to function 
candidates. If that's still the case, I think what we want is probably an 
option like `DisableSnippetTemplateForFunctionArgs`? 


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D50835



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

[PATCH] D50689: [clangd] NFC: Improve Dex Iterators debugging traits

[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-tools-extra/clangd/index/dex/Iterator.h:102
   /// ChildN is N-th iterator child. Raw iterators over PostingList are
-  /// represented as "[ID1, ID2, ...]" where IDN is N-th PostingList entry.
+  /// represented as "[ID1, ID2, ..., {IDX}, ... END]" where IDN is N-th
+  /// PostingList entry and IDX is the one currently being pointed to by the

nit: I think *IDX* might look a bit nicer. But up to you :)


https://reviews.llvm.org/D50689



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

[PATCH] D50337: [clangd] DexIndex implementation prototype

[Eric Liu via Phabricator via cfe-commits]

ioeric added a comment.

In https://reviews.llvm.org/D50337#1198914, @kbobyrev wrote:

> Don't separate the logic for "long" and "short" queries: 
> https://reviews.llvm.org/D50517 (https://reviews.llvm.org/rCTE339548) 
> introduced incomplete trigrams which can be used on for "short" queries, too.


Have you forgotten to upload the new revision? :)


https://reviews.llvm.org/D50337



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

[PATCH] D50700: [clangd] Generate better incomplete bigrams for the Dex index

[Eric Liu via Phabricator via cfe-commits]

ioeric added inline comments.



Comment at: clang-tools-extra/unittests/clangd/DexIndexTests.cpp:324
 
+  EXPECT_THAT(generateQueryTrigrams("u_p"), trigramsAre({"up$"}));
+  EXPECT_THAT(generateQueryTrigrams("_u_p"), trigramsAre({"_u_"}));

I'm not sure if this is correct. If users have explicitly typed `_`, they are 
likely to want a `_` there. You mentioned in the patch summary that users might 
want to match two heads with this. Could you provide an example?


https://reviews.llvm.org/D50700



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

[PATCH] D50700: [clangd] Generate better incomplete bigrams for the Dex index

[Eric Liu via Phabricator via cfe-commits]

ioeric added inline comments.



Comment at: clang-tools-extra/unittests/clangd/DexIndexTests.cpp:324
 
+  EXPECT_THAT(generateQueryTrigrams("u_p"), trigramsAre({"up$"}));
+  EXPECT_THAT(generateQueryTrigrams("_u_p"), trigramsAre({"_u_"}));

kbobyrev wrote:
> ioeric wrote:
> > I'm not sure if this is correct. If users have explicitly typed `_`, they 
> > are likely to want a `_` there. You mentioned in the patch summary that 
> > users might want to match two heads with this. Could you provide an example?
> The particular example I had on my mind was `unique_ptr`. Effectively, if the 
> query is `SC` then `StrCat` would be matched (because the incomplete trigram 
> would be `sc$` and two heads from the symbol identifier would also yield 
> `sc$`), but for `u_p`, `unique_ptr` is currently not matched.
> 
> On the other hand, if there's something like `m_field` and user types `mf` or 
> `m_f` it will be matched in both cases, because `m_field` yields `mf$` in the 
> index build stage, so this change doesn't decrease code completion quality 
> for such cases.
The problem is that `u_p` can now match `upXXX` with `up$`, which might be a 
bit surprising for users. 

It seems to me that another way way to handle this is to generate `u_p` trigram 
for `unique_ptr`. Have we considered including separators like `_` in trigrams? 


https://reviews.llvm.org/D50700



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

[PATCH] D50337: [clangd] DexIndex implementation prototype

[Eric Liu via Phabricator via cfe-commits]

ioeric added inline comments.



Comment at: clang-tools-extra/clangd/index/dex/DexIndex.cpp:25
+ std::vector> &Scores) {
+  // First, sort retrieved symbols by the cumulative score to apply callbacks
+  // in the order of descending score.

Why is this enforced? `fuzzyFind` doesn't say anything about callback order.

Also `useCallback` seems to be the right abstraction to me; different requests 
can have different callback behaviors. I think we could simply inline the code 
here.



Comment at: clang-tools-extra/clangd/index/dex/DexIndex.cpp:99
+  std::vector SymbolDocIDs;
+  // FIXME(kbobyrev): Scoring all matched symbols and then processing
+  // MaxCandidateCount items is rather expensive, this should be replaced by

Any reason we are not doing the two-stage scoring now? Retrieving while scoring 
with more expensive scoring seems to be diverging from the expected design. I 
think we can retrieve a relatively large number of symbols (e.g. a fixed large 
number or `100*MaxCandidateCount`?) before re-scoring them with more expensive 
scorers (e.g. fuzzy match), as consuming only `Req.MaxCandidateCount` symbols 
from the iterator tree can easily leave out good candidates (e.g. those that 
would've gotten good fuzzy match scores). 



Comment at: clang-tools-extra/clangd/index/dex/DexIndex.cpp:110
+// trigrams.
+const auto TrigramTokens = generateIdentifierTrigrams(Req.Query);
+std::vector> TrigramIterators;

It seems that the trigram generation could be done outside of the critical 
section?



Comment at: clang-tools-extra/clangd/index/dex/DexIndex.cpp:111
+const auto TrigramTokens = generateIdentifierTrigrams(Req.Query);
+std::vector> TrigramIterators;
+for (const auto &Trigram : TrigramTokens) {

I think we could pull some helper functions here to make the code a bit easier 
to follow e.g. `createTrigramIterators(TrigramTokens)`, 
`createScopeIterators(scopes)`...



Comment at: clang-tools-extra/clangd/index/dex/DexIndex.h:50
+
+  virtual void
+  lookup(const LookupRequest &Req,

Why virtual?



Comment at: clang-tools-extra/unittests/clangd/TestIndexOperations.h:1
+//===-- IndexHelpers.h --*- C++ 
-*-===//
+//

As this file contains helper functions for testing indexes, I'd suggest calling 
this `TestIndex.h` like `TestTU.h`.


https://reviews.llvm.org/D50337



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

[PATCH] D50847: [clangd] Add callbacks on parsed AST in addition to parsed preambles

[Eric Liu via Phabricator via cfe-commits]

ioeric added a comment.

> Dynamic index misses important information from the body of the file, e.g. 
> locations of definitions
>  XRefs cannot be collected at all, since we can only obtain full information 
> for the current file (preamble is parsed with skipped function bodies, 
> therefore not reliable).

These seem to be known limitations of the dynamic index. The dynamic index, in 
general, only handles things that are in headers; information in the main file 
can usually be obtained from Sema/AST (outside of the index). For example, 
definition/xrefs locations can be merged from index and the AST.

> This patch only adds the new callback, actually updating the index will be 
> added in a follow-up patch.

How do we plan to merge symbols from the preamble callback and the main file 
callback? The current FileIndex only allows swapping all symbols.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D50847



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

[PATCH] D50726: [clangd] Show function documentation in sigHelp

[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/CodeCompletionStrings.cpp:52
   // get this declaration, so we don't show documentation in that case.
   if (Result.Kind != CodeCompletionResult::RK_Declaration)
 return "";

`RK_Pattern` can also have declaration. Do we want to consider those?



Comment at: clangd/CodeCompletionStrings.cpp:57
+return "";
+  return getDeclComment(Ctx, *Decl);
+}

nit: maybe `return Decl ? getDeclComment(...) : "";`



Comment at: clangd/CodeCompletionStrings.cpp:77
 return "";
   return Doc;
 }

nit: `return looksLikeDocComment(Doc) ? Doc : "";` 


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D50726



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

[PATCH] D50726: [clangd] Show function documentation in sigHelp

[Eric Liu via Phabricator via cfe-commits]

ioeric added inline comments.



Comment at: clangd/CodeCompletionStrings.cpp:52
   // get this declaration, so we don't show documentation in that case.
   if (Result.Kind != CodeCompletionResult::RK_Declaration)
 return "";

ilya-biryukov wrote:
> ioeric wrote:
> > `RK_Pattern` can also have declaration. Do we want to consider those?
> See the FIXME at the top, we don't have the API exposed from clang to get the 
> proper declarations (IIRC, this only shows up for ObjC properties and we need 
> to look at both getters and setters when looking for a comment, and the clang 
> code does not expose the API to do that).
> 
> We should probably fix this, but that's a separate issue.
`getDeclaration()` has started supporting `RK_Pattern` recently, so this should 
be fixable now. But feel free to leave it to future.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D50726



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

[PATCH] D50337: [clangd] DexIndex implementation prototype

[Eric Liu via Phabricator via cfe-commits]

ioeric added inline comments.



Comment at: clang-tools-extra/clangd/index/dex/DexIndex.cpp:86
+  const auto TrigramTokens = generateIdentifierTrigrams(Req.Query);
+  TopLevelChildren.push_back(createAnd(createTrigramIterators(TrigramTokens)));
+

`createTrigramIterators` and `createScopeIterators` use `InvertedIndex`, so 
they should be called in the critical section. 

Maybe 

```
/// 
/// Requires: Called from a critical section of `InvertedIndex`.
std::vector> createTrigramIterators(
const llvm::DenseMap &InvertedIndex, TrigramTokens);
```



Comment at: clang-tools-extra/clangd/index/dex/DexIndex.cpp:102
+// final score might not be retrieved otherwise.
+const unsigned ItemsToRetrieve = 100 * Req.MaxCandidateCount;
+SymbolDocIDs = consume(*QueryIterator, ItemsToRetrieve);

Maybe add a `FIXME` about finding the best pre-scoring retrieval threshold. I'm 
not sure if `100*MaxCandidateCount` would work well in practice. For example, 
if the `MaxCandidateCount` is small e.g. `5`, then the retrieval threshold 
would only be 50, which still seems to be too small.



Comment at: clang-tools-extra/clangd/index/dex/DexIndex.cpp:110
+  const auto Score = Filter.match(Sym->Name);
+  assert(Score.hasValue() && "Matched symbol has all Fuzzy Matching "
+ "trigrams, it should match the query");

I don't think this assertion is well justified. I think we should just skip if 
the fuzzy match fails.



Comment at: clang-tools-extra/clangd/index/dex/DexIndex.cpp:118
+// in the order of descending score.
+std::sort(begin(Scores), end(Scores),
+  [](const std::pair &LHS,

I think we should use a priority queue so that we don't need to store/sort all 
retrieved symbols.



Comment at: clang-tools-extra/unittests/clangd/DexIndexTests.cpp:367
+// FIXME(kbobyrev): Use 3+ symbols long names so that trigram index is used.
+TEST(MergeDexIndex, Lookup) {
+  DexIndex I, J;

This seems to be testing `mergeIndex(...)` which is not relevant in this patch? 



Comment at: clang-tools-extra/unittests/clangd/DexIndexTests.cpp:388
+
+// FIXME(kbobyrev): Add more tests on DexIndex? Right now, it's mostly a 
wrapper
+// around DexIndex, adopting tests from IndexTests.cpp sounds reasonable.

Now that we are handling both short and long queries. I think we can address 
this FIXME in this patch?



Comment at: clang-tools-extra/unittests/clangd/DexIndexTests.cpp:540
+
+TEST(DexMergeIndexTest, Lookup) {
+  DexIndex I, J;

Again, `mergeIndex(...)` is not interesting here.


https://reviews.llvm.org/D50337



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

[PATCH] D50835: [clangd] Add parantheses while auto-completing functions.

[Eric Liu via Phabricator via cfe-commits]

ioeric added inline comments.



Comment at: clangd/CodeComplete.cpp:1377
+  // Check whether function has any parameters or not.
+  LSP.textEdit->newText += SnippetSuffix.size() > 2 ? "(${0})" : "()";
+else

There seems to be no guarantee on whether the snippet is function arg template 
when `SnippetSuffix.size() > 2`. A heuristic we could use is probably checking 
that the template starts with `(` and ends with `)`? Alternatively, we could 
try to avoid generating the proper snippet according to the flag when we still 
have the function declaration around. Not sure if this is feasible for index 
results though.



Comment at: clangd/CodeComplete.h:85
+
+  /// Enables cursor to be moved around according to completions needs even 
when
+  /// snippets are disabled. For example selecting a function with parameters 
as

ilya-biryukov wrote:
> kadircet wrote:
> > ioeric wrote:
> > > IIRC, the goal of this patch is to allow disabling snippet templates for 
> > > function parameters while still allow appending `()` or `($0)` to 
> > > function candidates. If that's still the case, I think what we want is 
> > > probably an option like `DisableSnippetTemplateForFunctionArgs`? 
> > Yeah, that makes totally more sense. Thanks!
> - Maybe use positive option names to avoid double negation?
> - Maybe also use a shorter name? E.g. removing 'template' and 'disable' from 
> the name gives a name that does not look any worse: `FunctionArgSnippets`.
+1 to a better name. I didn't really give much thought about the name and just 
wanted to get the idea across :)


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D50835



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

[PATCH] D50337: [clangd] DexIndex implementation prototype

[Eric Liu via Phabricator via cfe-commits]

ioeric added a comment.

Almost LG! Just a few more nits.




Comment at: clang-tools-extra/clangd/index/dex/DexIndex.cpp:87
+  std::vector SymbolDocIDs;
+  std::priority_queue> Top;
+

nit: move `SymbolDocIDs` and `Top` closer to where they're used.



Comment at: clang-tools-extra/clangd/index/dex/DexIndex.cpp:97
+// Add OR iterator for scopes if the request contains scopes.
+if (!Req.Scopes.empty()) {
+  TopLevelChildren.push_back(createScopeIterator(Req.Scopes));

I think we should let `createScopeIterator` handle empty scope list case; it 
can return an empty list anyway.



Comment at: clang-tools-extra/clangd/index/dex/DexIndex.cpp:109
+SymbolDocIDs = consume(*QueryIterator, ItemsToRetrieve);
+More = SymbolDocIDs.size() >= Req.MaxCandidateCount;
+

This is not a proper place to set `More`. It's already handled below.



Comment at: clang-tools-extra/clangd/index/dex/DexIndex.cpp:112
+// Populate scores.
+for (size_t SymbolIdx = 0; SymbolIdx < SymbolDocIDs.size(); ++SymbolIdx) {
+  const auto Sym = (*Symbols)[SymbolDocIDs[SymbolIdx]];

nit: use range-based for loop?



Comment at: clang-tools-extra/clangd/index/dex/DexIndex.cpp:113
+for (size_t SymbolIdx = 0; SymbolIdx < SymbolDocIDs.size(); ++SymbolIdx) {
+  const auto Sym = (*Symbols)[SymbolDocIDs[SymbolIdx]];
+  const auto Score = Filter.match(Sym->Name);

nit: Maybe take `const auto &Sym`?



Comment at: clang-tools-extra/clangd/index/dex/DexIndex.cpp:119
+  // highest actual score.
+  Top.emplace(-*Score * SymbolQuality.find(Sym)->second, Sym);
+  if (Top.size() > Req.MaxCandidateCount) {

nit: `- (*Score) * ...` for readability.



Comment at: clang-tools-extra/clangd/index/dex/DexIndex.cpp:127
+// Apply callback to the top Req.MaxCandidateCount items.
+for (; !Top.empty(); Top.pop())
+  Callback(*Top.top().second);

Can we simply iterate without `pop()`?



Comment at: clang-tools-extra/clangd/index/dex/DexIndex.cpp:150
+std::unique_ptr
+DexIndex::createTrigramIterator(std::vector TrigramTokens) const {
+  std::vector> TrigramIterators;

This assumes that `createTrigramIterator` and `createScopeIterator` are already 
guarded by the mutex, which is implicit. I think we can make it clearer by 
making these local helpers that take in InvertedIndex` with the requirement 
that local has been acquired.



Comment at: clang-tools-extra/unittests/clangd/DexIndexTests.cpp:366
+
+// FIXME(kbobyrev): Add more tests on DexIndex? Right now, it's mostly a 
wrapper
+// around DexIndex, adopting tests from IndexTests.cpp sounds reasonable.

What tests do we want? If it's related to the changes in this patch, we should 
add it now. Tests shouldn't be `FIXME` :)


https://reviews.llvm.org/D50337



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

[PATCH] D50835: [clangd] Add parantheses while auto-completing functions.

[Eric Liu via Phabricator via cfe-commits]

ioeric added inline comments.



Comment at: clangd/CodeComplete.cpp:1377
+  // Check whether function has any parameters or not.
+  LSP.textEdit->newText += SnippetSuffix.size() > 2 ? "(${0})" : "()";
+else

kadircet wrote:
> ioeric wrote:
> > There seems to be no guarantee on whether the snippet is function arg 
> > template when `SnippetSuffix.size() > 2`. A heuristic we could use is 
> > probably checking that the template starts with `(` and ends with `)`? 
> > Alternatively, we could try to avoid generating the proper snippet 
> > according to the flag when we still have the function declaration around. 
> > Not sure if this is feasible for index results though.
> Actually I was first trying to directly look at function argument count as 
> you mentioned, but unfortunately Symbols coming from index don't have any 
> declarations. Therefore it turns back to looking at Signature in this case, 
> which is same with this one. Apart from that, IIUC, SnippetSuffix contains 
> anything only when completion item is a function otherwise it is empty. So 
> IMO this check should be OK.
> Apart from that, IIUC, SnippetSuffix contains anything only when completion 
> item is a function otherwise it is empty. So IMO this check should be OK.
This seems to be true for the current use cases, but I suspect the assumption 
can change when we add more snippet-based completion results in the future e.g. 
a snippet for if-statement `if (${0}) {${1}}`.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D50835



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

[PATCH] D50337: [clangd] DexIndex implementation prototype

[Eric Liu via Phabricator via cfe-commits]

ioeric added inline comments.



Comment at: clang-tools-extra/clangd/index/dex/DexIndex.cpp:97
+}
+// FIXME(kbobyrev): Add True iterator as soon as it's implemented 
otherwise.
+// If TopLevelChildren vector will be empty it will trigger an assertion.

As discussed offline, triggering assertion seems to be a pretty bad behavior. 
Although the trigram generation, as you suggested, always more than one token, 
we should try to get rid of this FIXME by introducing the true iterator as 
proposed here.



Comment at: clang-tools-extra/clangd/index/dex/DexIndex.cpp:121
+std::vector SymbolDocIDs = consume(*QueryIterator, ItemsToRetrieve);
+More = SymbolDocIDs.size() >= Req.MaxCandidateCount;
+

Still, we shouldn't set `More` here.



Comment at: clang-tools-extra/clangd/index/dex/DexIndex.h:58
+private:
+  std::shared_ptr> Symbols;
+  llvm::DenseMap LookupTable;

nit: add /*GUARDED_BY(Mutex)*/



Comment at: clang-tools-extra/unittests/clangd/DexIndexTests.cpp:366
+
+TEST(DexIndex, FuzzyFind) {
+  DexIndex Index;

Please add tests with empty `Query`.


https://reviews.llvm.org/D50337



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

[PATCH] D50835: [clangd] Add parantheses while auto-completing functions.

[Eric Liu via Phabricator via cfe-commits]

ioeric added inline comments.



Comment at: clangd/CodeComplete.cpp:1379
+  Kind == CompletionItemKind::Method) &&
+ SnippetSuffix.front() == '(' && SnippetSuffix.back() == ')') {
+  // Check whether function has any parameters or not.

nit: add parentheses around `(... == ...)`



Comment at: clangd/CodeComplete.cpp:1380
+ SnippetSuffix.front() == '(' && SnippetSuffix.back() == ')') {
+  // Check whether function has any parameters or not.
+  LSP.textEdit->newText += SnippetSuffix.size() > 2 ? "(${0})" : "()";

nit: no braces around one liner.



Comment at: clangd/CodeComplete.cpp:1381
+  // Check whether function has any parameters or not.
+  LSP.textEdit->newText += SnippetSuffix.size() > 2 ? "(${0})" : "()";
+}

I think we are missing one case here: `!EnableFunctionArgSnippets && ()`?

I think we could do something like:
```
if (!EnableFunctionArgSnippets && ())
  // Truncate parameter template
else
  // append snippet like before

```


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D50835



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

[PATCH] D50835: [clangd] Add parantheses while auto-completing functions.

[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


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D50835



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

[PATCH] D50896: [clangd] Add xrefs LSP boilerplate implementation.

[Eric Liu via Phabricator via cfe-commits]

ioeric added a comment.

Agreed with Ilya. I'd probably also make this depend on the ongoing 
implementation, as exposing LSP endpoints without proper implementation might 
be confusing to clangd users who only look at the the LSP endpoints. Users need 
to dig two levels of abstraction to find out that it's not implemented.




Comment at: clangd/ClangdServer.h:158
+  /// Retrieve locations for symbol references.
+  void references(PathRef File, Position Pos, bool includeDeclaration,
+  Callback> CB);

I think the C++ API can return `SymbolOccurrence` in the callback, which would 
allow C++ API users to get richer information about the occurrences e.g. kind, 
relationship, code snippet.



Comment at: clangd/ClangdServer.h:158
+  /// Retrieve locations for symbol references.
+  void references(PathRef File, Position Pos, bool includeDeclaration,
+  Callback> CB);

ioeric wrote:
> I think the C++ API can return `SymbolOccurrence` in the callback, which 
> would allow C++ API users to get richer information about the occurrences 
> e.g. kind, relationship, code snippet.
nit: s/includeDeclaration/IncludeDeclaration/


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D50896



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

[PATCH] D50955: [clangd] Implement TRUE Iterator

[Eric Liu via Phabricator via cfe-commits]

ioeric added inline comments.



Comment at: clang-tools-extra/clangd/index/dex/Iterator.cpp:230
+/// order to prevent additional memory consumption, it only stores the size of
+/// this virtual posting list because posting lists of such density are likely
+/// to consume a lot of memory. All operations are performed in O(1) as a

nit: The documentation here is a bit verbose as. How about `... It stores size 
of the virtual posting list, and all operations are performed in O(1).`? Other 
statements can be easily derived from this IMO.



Comment at: clang-tools-extra/clangd/index/dex/Iterator.cpp:248
+assert(!reachedEnd() && "Can't advance iterator after it reached the 
end.");
+Index = ID;
+  }

Should we check `ID < Size` here?



Comment at: clang-tools-extra/clangd/index/dex/Iterator.cpp:258
+  llvm::raw_ostream &dump(llvm::raw_ostream &OS) const override {
+OS << "(TrueIterator {" << Index << "} out of " << Size << ")";
+return OS;

nit: maybe `s/TrueIterator/TRUE/`?


https://reviews.llvm.org/D50955



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

[PATCH] D50956: [clangd] NFC: Cleanup Dex Iterator comments and simplify tests

[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


https://reviews.llvm.org/D50956



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

[PATCH] D50955: [clangd] Implement TRUE Iterator

[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


https://reviews.llvm.org/D50955



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

[PATCH] D50847: [clangd] Add callbacks on parsed AST in addition to parsed preambles

[Eric Liu via Phabricator via cfe-commits]

ioeric added inline comments.
Herald added a subscriber: kadircet.



Comment at: clangd/TUScheduler.h:66
+  /// instead.
+  virtual void onMainAST(PathRef Path, ParsedAST &AST) = 0;
+};

ilya-biryukov wrote:
> hokein wrote:
> > Does the callback get called every time we built an AST? clangd only has 3 
> > idle ASTs, if the AST is not there, we rebuild it when needed (even the 
> > source code is not changed), and we will update the dynamic index, which 
> > seems unnecessary.
> > 
> > It may rarely happen, 3 idle ASTs  might cover most cases, I think? 
> Currently we only call this when AST is built for diagnostics, so we will 
> have only a single callback, even if the AST will be rebuilt multiple times 
> because it was flushed out of the cash (as long as the file contents didn't 
> change, of course).
> 
> So we do behave optimally in that case and I suggest we keep it this way
is there any overlap between preamble AST and main AST? If so, could you 
elaborate in the documentation? 


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D50847



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

[PATCH] D50960: [clangd] Add missing lock in the lookup.

[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


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D50960



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

[PATCH] D50961: [clangd] Simplify the code using UniqueStringSaver, NFC.

[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: docs/clang-tidy/checks/abseil-duration-division.rst:1
+.. title:: clang-tidy - abseil-duration-division
+

Please revert.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D50961



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

[PATCH] D50962: [clangd] Speculative code completion index request before Sema is run.

[Eric Liu via Phabricator via cfe-commits]

ioeric created this revision.
ioeric added reviewers: hokein, ilya-biryukov.
Herald added subscribers: cfe-commits, kadircet, arphaman, jkorous, MaskRay.

For index-based code completion, send an asynchronous speculative index
request, based on the index request for the last code completion on the same
file and the filter text typed before the cursor, before sema code completion
is invoked. This can reduce the code completion latency (by roughly latency of
sema code completion) if the speculative request is the same as the one
generated for the ongoing code completion from sema. As a sequence of code
completions often have the same scopes and proximity paths etc, this should be
effective for a number of code completions.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D50962

Files:
  clangd/ClangdServer.cpp
  clangd/ClangdServer.h
  clangd/CodeComplete.cpp
  clangd/CodeComplete.h
  clangd/index/Index.cpp
  clangd/index/Index.h
  clangd/tool/ClangdMain.cpp
  unittests/clangd/CodeCompleteTests.cpp

Index: unittests/clangd/CodeCompleteTests.cpp
===
--- unittests/clangd/CodeCompleteTests.cpp
+++ unittests/clangd/CodeCompleteTests.cpp
@@ -1594,6 +1594,66 @@
   ElementsAre(Sig("foo(T, U) -> void", {"T", "U"})));
 }
 
+TEST(SpeculateCompletionFilter, Filters) {
+  Annotations F(R"cpp($bof^
+  $bol^
+  ab$ab^
+  x.ab$dot^
+  x.$dotempty^
+  x::ab$scoped^
+  x::$scopedempty^
+
+  )cpp");
+  auto speculate = [&](StringRef PointName) {
+auto Filter = speculateCompletionFilter(F.code(), F.point(PointName));
+assert(Filter);
+return *Filter;
+  };
+  EXPECT_EQ(speculate("bof"), "");
+  EXPECT_EQ(speculate("bol"), "");
+  EXPECT_EQ(speculate("dot"), "ab");
+  EXPECT_EQ(speculate("dotempty"), "");
+  EXPECT_EQ(speculate("scoped"), "ab");
+  EXPECT_EQ(speculate("scopedempty"), "");
+}
+
+TEST(CompletionTest, EnableSpeculativeIndexRequest) {
+  MockFSProvider FS;
+  MockCompilationDatabase CDB;
+  IgnoreDiagnostics DiagConsumer;
+  ClangdServer Server(CDB, FS, DiagConsumer, ClangdServer::optsForTest());
+
+  FS.Files[testPath("bar.h")] =
+  R"cpp(namespace ns { struct preamble { int member; }; })cpp";
+  auto File = testPath("foo.cpp");
+  Annotations Test(R"cpp(
+  #include "bar.h"
+  namespace ns1 { int local1; }
+  namespace ns2 { int local2; }
+  void f() { ns1::^; }
+  void f() { ns2::$2^; }
+  )cpp");
+  runAddDocument(Server, File, Test.code());
+  clangd::CodeCompleteOptions Opts = {};
+
+  auto I = memIndex({var("ns1::index1"), var("ns2::index2")});
+  Opts.Index = I.get();
+  Opts.SpeculativeIndexRequest = true;
+
+  auto First = cantFail(runCodeComplete(Server, File, Test.point(), Opts));
+  EXPECT_THAT(First.Completions,
+  UnorderedElementsAre(Named("local1"), Named("index1")));
+
+  auto Second = cantFail(runCodeComplete(Server, File, Test.point(), Opts));
+  EXPECT_THAT(Second.Completions,
+  UnorderedElementsAre(Named("local1"), Named("index1")));
+
+  auto CacheMiss =
+  cantFail(runCodeComplete(Server, File, Test.point("2"), Opts));
+  EXPECT_THAT(CacheMiss.Completions,
+  UnorderedElementsAre(Named("local2"), Named("index2")));
+}
+
 } // namespace
 } // namespace clangd
 } // namespace clang
Index: clangd/tool/ClangdMain.cpp
===
--- clangd/tool/ClangdMain.cpp
+++ clangd/tool/ClangdMain.cpp
@@ -164,6 +164,20 @@
"an include line will be inserted or not."),
 llvm::cl::init(true));
 
+static llvm::cl::opt SpeculateCompletionIndexRequest(
+"speculative-completion-index-request",
+llvm::cl::desc(
+R"(If set to true and static index is enabled, this will send an
+asynchronous speculative index request, based on the index request for
+the last code completion on the same file and the filter text typed
+before the cursor, before sema code completion is invoked. This can
+reduce the code completion latency (by roughly latency of sema code
+completion) if the speculative request is the same as the one generated
+for the ongoing code completion from sema. As a sequence of code
+completions often have the same scopes and proximity paths etc, this
+should be effective for a number of code completions.)"),
+llvm::cl::init(true));
+
 static llvm::cl::opt YamlSymbolFile(
 "yaml-symbol-file",
 llvm::cl::desc(
@@ -299,6 +313,8 @@
 CCOpts.IncludeIndicator.Insert.clear();
 CCOpts.IncludeIndicator.NoInsert.clear();
   }
+  CCOpts.SpeculativeIndexRequest =
+  SpeculateCompletionIndexRequest && Opts.StaticIndex;
 
   // Initialize and run ClangdLSPServer.
   ClangdLSPServer LSPServer(
Index: clangd/index/Index.h
===
--- clangd/index/Index.h
+++ clangd/index/Index.h
@@ -18,7 +18,10 @@
 

[PATCH] D50889: [clangd] Make FileIndex aware of the main file

[Eric Liu via Phabricator via cfe-commits]

ioeric added inline comments.



Comment at: clangd/ClangdServer.cpp:70
+// FIXME(ibiryukov): this should be a generic helper instead.
+class NoopCallbacks : public ParsingCallbacks {
 public:

Maybe provide noop implementations for `ParsingCallbacks::onPreambleAST()` and 
`ParsingCallbacks::onMainAST` by default?



Comment at: clangd/ClangdServer.h:213
+  /// from the file itself. Those have different lifetimes and we merge 
results from both 
+  class DynamicIndex : public ParsingCallbacks {
+  public:

Can we move this into ClangdServer.cpp as an implementation helper? 



Comment at: clangd/ClangdServer.h:246
+  /// If present, an up-to-date of symbols in open files. Read via Index.
+  std::unique_ptr FileIdx;
   // If present, a merged view of FileIdx and an external index. Read via 
Index.

nit: `s/FileIdx/DymIdx`? (also need to update the comment above)



Comment at: clangd/index/FileIndex.cpp:46
+   AST.getTranslationUnitDecl()->decls().end());
+TopLevelDecls = Storage;
+  }

nit: I'd try avoiding modify the input argument if possible. Maybe set 
`Storage` properly according to `TopLevelDecls`?


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D50889



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

[PATCH] D50926: [SourceManager] Extract 'findFileIDsForFile' from 'translateFile' to allow mapping from one file entry to multiple FileIDs

[Eric Liu via Phabricator via cfe-commits]

ioeric added inline comments.



Comment at: include/clang/Basic/SourceManager.h:1533
 
+  /// Looks through all the local and imported source locations to find the set
+  /// of FileIDs that correspond to the given entry.

nit: put this closer to the closely related `translateFile`?



Comment at: include/clang/Basic/SourceManager.h:1539
+  /// \returns true if the callback returned true, false otherwise.
+  bool findFileIDsForFile(const FileEntry *SourceFile,
+  llvm::function_ref Callback) const;

Callback pattern seems uncommon in LLVM/Clang. I'd suggest making this return a 
set of `FileID`s and put the callback-based function as a helper (shared by 
this and `translateFile`)in the implementation.


Repository:
  rC Clang

https://reviews.llvm.org/D50926



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

[PATCH] D50502: [clangd] Initial cancellation mechanism for LSP requests.

[Eric Liu via Phabricator via cfe-commits]

ioeric added inline comments.



Comment at: clangd/Cancellation.h:96
+/// checks using it to avoid extra lookups in the Context.
+class CancellationToken {
+public:

As chatted offline, I have questions about the motivation of the 
`CancellationToken` class. Intuitively, it seems to me that this can be merged 
into `TaskHandle`, and we can simply stash the `TaskHandle` instead of a token 
into the context. There would be fewer states to maintain, and the design would 
be a bit more straightforward. I might be missing context/concerns here, so I'd 
like to hear what you and Ilya think. @ilya-biryukov 



Comment at: clangd/ClangdLSPServer.cpp:75
+std::string NormalizeRequestID(const json::Value &ID) {
+  CancelParams CP;
+  fromJSON(json::Object{{"id", ID}}, CP);

Use `ID.getAsString()`?


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D50502



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

[PATCH] D50337: [clangd] DexIndex implementation prototype

[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. Last few nits and then good to go.




Comment at: clang-tools-extra/clangd/index/dex/DexIndex.cpp:97
+}
+TopLevelChildren.push_back(createAnd(move(TrigramIterators)));
+

Check `!TrigramIterators.empty()`?



Comment at: clang-tools-extra/clangd/index/dex/DexIndex.cpp:128
+  const llvm::Optional Score = Filter.match(Sym->Name);
+  if (!Score.hasValue())
+continue;

nit: `if (!Score)`



Comment at: clang-tools-extra/clangd/index/dex/DexIndex.cpp:149
+  llvm::function_ref Callback) const 
{
+  for (const auto &ID : Req.IDs) {
+auto I = LookupTable.find(ID);

This also needs lock.



Comment at: clang-tools-extra/unittests/clangd/TestIndex.h:23
+
+Symbol symbol(llvm::StringRef QName);
+

Add comment about what `SymbolID` is?



Comment at: clang-tools-extra/unittests/clangd/TestIndex.h:25
+
+struct SlabAndPointers {
+  SymbolSlab Slab;

Please add documentation.



Comment at: clang-tools-extra/unittests/clangd/TestIndex.h:46
+
+std::vector match(const SymbolIndex &I,
+   const FuzzyFindRequest &Req,

Please add documentation.


https://reviews.llvm.org/D50337



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

[PATCH] D50966: Fix an undefined behavior when storing an empty StringRef.

[Eric Liu via Phabricator via cfe-commits]

ioeric added inline comments.



Comment at: lib/Support/StringSaver.cpp:14
 
 StringRef StringSaver::save(StringRef S) {
+  if (S.empty())

Is it possible to reuse `StringRef::copy(Allocator &)` here?


Repository:
  rL LLVM

https://reviews.llvm.org/D50966



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

[PATCH] D50897: [clangd] Allow using experimental Dex index

[Eric Liu via Phabricator via cfe-commits]

ioeric added inline comments.
Herald added a subscriber: kadircet.



Comment at: clang-tools-extra/clangd/index/Index.h:360
+ template 
+ static std::unique_ptr build(SymbolSlab Slab) {
+   struct Snapshot {

I'd try to avoid this pattern as it implicitly requires 
`T::build(std::shared_ptr>)` to be implemented. 
Alternatively, you could pull out a helper function that turns a `SymbolSlab` 
into `std::shared_ptr>`, which can be shared by 
different indexes. It can probably live in `MemIndex.h`.


https://reviews.llvm.org/D50897



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

[PATCH] D50966: Fix an undefined behavior when storing an empty StringRef.

[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




Comment at: lib/Support/StringSaver.cpp:14
 
 StringRef StringSaver::save(StringRef S) {
+  if (S.empty())

hokein wrote:
> ioeric wrote:
> > Is it possible to reuse `StringRef::copy(Allocator &)` here?
> Unfortunately, not, `StringRef::copy` will change the behavior. The 
> `StringRef` returned here is null-terminated.
Hmm, that seems strange but out of the scope.


Repository:
  rL LLVM

https://reviews.llvm.org/D50966



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

[PATCH] D50962: [clangd] Speculative code completion index request before Sema is run.

[Eric Liu via Phabricator via cfe-commits]

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

I just realized that `CodeCompleteFlow` would be blocked by the speculative 
index request even when index request is not needed (e.g. member access), 
because it owns the future object. This can make some completion requests 
slower. We may need to move the async index query into the 
`ClangdServer::codeComplete` so that completion callback can be invoked as soon 
as results are ready, without having to wait for speculative index results.

F7000668: future-redundant-wait.png 


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D50962



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

[PATCH] D50962: [clangd] Speculative code completion index request before Sema is run.

[Eric Liu via Phabricator via cfe-commits]

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

- Make sure completion result callback can be called even if the unused 
speculative request has not finished.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D50962

Files:
  clangd/ClangdServer.cpp
  clangd/ClangdServer.h
  clangd/CodeComplete.cpp
  clangd/CodeComplete.h
  clangd/index/Index.cpp
  clangd/index/Index.h
  clangd/tool/ClangdMain.cpp
  unittests/clangd/CodeCompleteTests.cpp
  unittests/clangd/IndexTests.cpp

Index: unittests/clangd/IndexTests.cpp
===
--- unittests/clangd/IndexTests.cpp
+++ unittests/clangd/IndexTests.cpp
@@ -321,6 +321,21 @@
   EXPECT_EQ(M.Name, "right");
 }
 
+TEST(AsyncFuzzyFind, Simple) {
+  MemIndex Idx;
+  Idx.build(generateSymbols({"ns::abc", "ns::xyz"}));
+
+  FuzzyFindRequest Req;
+  Req.Query = "";
+  Req.Scopes = {"ns::"};
+
+  AsyncFuzzyFind Async(Idx, Req);
+
+  EXPECT_EQ(Async.getRequest(), Req);
+  EXPECT_THAT(Async.getResult(),
+  UnorderedElementsAre(Named("abc"), Named("xyz")));
+}
+
 } // namespace
 } // namespace clangd
 } // namespace clang
Index: unittests/clangd/CodeCompleteTests.cpp
===
--- unittests/clangd/CodeCompleteTests.cpp
+++ unittests/clangd/CodeCompleteTests.cpp
@@ -923,7 +923,11 @@
llvm::function_ref
Callback) const override {}
 
-  const std::vector allRequests() const { return Requests; }
+  const std::vector consumeRequests() const {
+auto Reqs = std::move(Requests);
+Requests.clear();
+return Reqs;
+  }
 
 private:
   mutable std::vector Requests;
@@ -934,7 +938,7 @@
   IndexRequestCollector Requests;
   Opts.Index = &Requests;
   completions(Code, {}, Opts);
-  return Requests.allRequests();
+  return Requests.consumeRequests();
 }
 
 TEST(CompletionTest, UnqualifiedIdQuery) {
@@ -1700,6 +1704,71 @@
   }
 }
 
+TEST(SpeculateCompletionFilter, Filters) {
+  Annotations F(R"cpp($bof^
+  $bol^
+  ab$ab^
+  x.ab$dot^
+  x.$dotempty^
+  x::ab$scoped^
+  x::$scopedempty^
+
+  )cpp");
+  auto speculate = [&](StringRef PointName) {
+auto Filter = speculateCompletionFilter(F.code(), F.point(PointName));
+assert(Filter);
+return *Filter;
+  };
+  EXPECT_EQ(speculate("bof"), "");
+  EXPECT_EQ(speculate("bol"), "");
+  EXPECT_EQ(speculate("dot"), "ab");
+  EXPECT_EQ(speculate("dotempty"), "");
+  EXPECT_EQ(speculate("scoped"), "ab");
+  EXPECT_EQ(speculate("scopedempty"), "");
+}
+
+TEST(CompletionTest, EnableSpeculativeIndexRequest) {
+  MockFSProvider FS;
+  MockCompilationDatabase CDB;
+  IgnoreDiagnostics DiagConsumer;
+  ClangdServer Server(CDB, FS, DiagConsumer, ClangdServer::optsForTest());
+
+  auto File = testPath("foo.cpp");
+  Annotations Test(R"cpp(
+  namespace ns1 { int abc; }
+  namespace ns2 { int abc; }
+  void f() { ns1::ab$1^; ns1::ab$2^; }
+  void f() { ns2::ab$3^; }
+  )cpp");
+  runAddDocument(Server, File, Test.code());
+  clangd::CodeCompleteOptions Opts = {};
+
+  IndexRequestCollector Requests;
+  Opts.Index = &Requests;
+  Opts.SpeculativeIndexRequest = true;
+
+  cantFail(runCodeComplete(Server, File, Test.point("1"), Opts));
+  auto Reqs1 = Requests.consumeRequests();
+  ASSERT_EQ(Reqs1.size(), 1u);
+  EXPECT_THAT(Reqs1[0].Scopes, UnorderedElementsAre("ns1::"));
+
+  // Speculation succeeded. Use speculative index result.
+  cantFail(runCodeComplete(Server, File, Test.point("2"), Opts));
+  auto Reqs2 = Requests.consumeRequests();
+  ASSERT_EQ(Reqs2.size(), 1u);
+  EXPECT_EQ(Reqs2[0], Reqs1[0]);
+
+  cantFail(runCodeComplete(Server, File, Test.point("3"), Opts));
+  // Sleep for a while to make sure asynchronous call is also triggered before
+  // callback is invoked.
+  std::this_thread::sleep_for(std::chrono::milliseconds(100));
+
+  // Speculation failed. Sent speculative index request and the new index
+  // request after sema.
+  auto Reqs3 = Requests.consumeRequests();
+  ASSERT_EQ(Reqs3.size(), 2u);
+}
+
 } // namespace
 } // namespace clangd
 } // namespace clang
Index: clangd/tool/ClangdMain.cpp
===
--- clangd/tool/ClangdMain.cpp
+++ clangd/tool/ClangdMain.cpp
@@ -164,6 +164,20 @@
"an include line will be inserted or not."),
 llvm::cl::init(true));
 
+static llvm::cl::opt SpeculateCompletionIndexRequest(
+"speculative-completion-index-request",
+llvm::cl::desc(
+R"(If set to true and static index is enabled, this will send an
+asynchronous speculative index request, based on the index request for
+the last code completion on the same file and the filter text typed
+before the cursor, before sema code completion is invoked. This can
+reduce the code completion latency (by roughly latency of sema code
+completion) if the speculative requ

[PATCH] D50962: [clangd] Speculative code completion index request before Sema is run.

[Eric Liu via Phabricator via cfe-commits]

ioeric updated this revision to Diff 161651.
ioeric marked an inline comment as done.
ioeric added a comment.

- Improve tests.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D50962

Files:
  clangd/ClangdServer.cpp
  clangd/ClangdServer.h
  clangd/CodeComplete.cpp
  clangd/CodeComplete.h
  clangd/index/Index.cpp
  clangd/index/Index.h
  clangd/tool/ClangdMain.cpp
  unittests/clangd/CodeCompleteTests.cpp
  unittests/clangd/IndexTests.cpp

Index: unittests/clangd/IndexTests.cpp
===
--- unittests/clangd/IndexTests.cpp
+++ unittests/clangd/IndexTests.cpp
@@ -321,6 +321,21 @@
   EXPECT_EQ(M.Name, "right");
 }
 
+TEST(AsyncFuzzyFind, Simple) {
+  MemIndex Idx;
+  Idx.build(generateSymbols({"ns::abc", "ns::xyz"}));
+
+  FuzzyFindRequest Req;
+  Req.Query = "";
+  Req.Scopes = {"ns::"};
+
+  AsyncFuzzyFind Async(Idx, Req);
+
+  EXPECT_EQ(Async.getRequest(), Req);
+  EXPECT_THAT(Async.getResult(),
+  UnorderedElementsAre(Named("abc"), Named("xyz")));
+}
+
 } // namespace
 } // namespace clangd
 } // namespace clang
Index: unittests/clangd/CodeCompleteTests.cpp
===
--- unittests/clangd/CodeCompleteTests.cpp
+++ unittests/clangd/CodeCompleteTests.cpp
@@ -923,7 +923,11 @@
llvm::function_ref
Callback) const override {}
 
-  const std::vector allRequests() const { return Requests; }
+  const std::vector consumeRequests() const {
+auto Reqs = std::move(Requests);
+Requests.clear();
+return Reqs;
+  }
 
 private:
   mutable std::vector Requests;
@@ -934,7 +938,7 @@
   IndexRequestCollector Requests;
   Opts.Index = &Requests;
   completions(Code, {}, Opts);
-  return Requests.allRequests();
+  return Requests.consumeRequests();
 }
 
 TEST(CompletionTest, UnqualifiedIdQuery) {
@@ -1700,6 +1704,75 @@
   }
 }
 
+TEST(SpeculateCompletionFilter, Filters) {
+  Annotations F(R"cpp($bof^
+  $bol^
+  ab$ab^
+  x.ab$dot^
+  x.$dotempty^
+  x::ab$scoped^
+  x::$scopedempty^
+
+  )cpp");
+  auto speculate = [&](StringRef PointName) {
+auto Filter = speculateCompletionFilter(F.code(), F.point(PointName));
+assert(Filter);
+return *Filter;
+  };
+  EXPECT_EQ(speculate("bof"), "");
+  EXPECT_EQ(speculate("bol"), "");
+  EXPECT_EQ(speculate("ab"), "ab");
+  EXPECT_EQ(speculate("dot"), "ab");
+  EXPECT_EQ(speculate("dotempty"), "");
+  EXPECT_EQ(speculate("scoped"), "ab");
+  EXPECT_EQ(speculate("scopedempty"), "");
+}
+
+TEST(CompletionTest, EnableSpeculativeIndexRequest) {
+  MockFSProvider FS;
+  MockCompilationDatabase CDB;
+  IgnoreDiagnostics DiagConsumer;
+  ClangdServer Server(CDB, FS, DiagConsumer, ClangdServer::optsForTest());
+
+  auto File = testPath("foo.cpp");
+  Annotations Test(R"cpp(
+  namespace ns1 { int abc; }
+  namespace ns2 { int abc; }
+  void f() { ns1::ab$1^; ns1::ab$2^; }
+  void f() { ns2::ab$3^; }
+  )cpp");
+  runAddDocument(Server, File, Test.code());
+  clangd::CodeCompleteOptions Opts = {};
+
+  IndexRequestCollector Requests;
+  Opts.Index = &Requests;
+  Opts.SpeculativeIndexRequest = true;
+
+  auto CompleteAtPoint = [&](StringRef P) {
+cantFail(runCodeComplete(Server, File, Test.point(P), Opts));
+// Sleep for a while to make sure asynchronous call (if applicable) is also
+// triggered before callback is invoked.
+std::this_thread::sleep_for(std::chrono::milliseconds(100));
+  };
+
+  CompleteAtPoint("1");
+  auto Reqs1 = Requests.consumeRequests();
+  ASSERT_EQ(Reqs1.size(), 1u);
+  EXPECT_THAT(Reqs1[0].Scopes, UnorderedElementsAre("ns1::"));
+
+  CompleteAtPoint("2");
+  auto Reqs2 = Requests.consumeRequests();
+  // Speculation succeeded. Used speculative index result.
+  ASSERT_EQ(Reqs2.size(), 1u);
+  EXPECT_EQ(Reqs2[0], Reqs1[0]);
+
+  CompleteAtPoint("3");
+  // Speculation failed. Sent speculative index request and the new index
+  // request after sema.
+  auto Reqs3 = Requests.consumeRequests();
+  ASSERT_EQ(Reqs3.size(), 2u);
+}
+
 } // namespace
 } // namespace clangd
 } // namespace clang
Index: clangd/tool/ClangdMain.cpp
===
--- clangd/tool/ClangdMain.cpp
+++ clangd/tool/ClangdMain.cpp
@@ -164,6 +164,20 @@
"an include line will be inserted or not."),
 llvm::cl::init(true));
 
+static llvm::cl::opt SpeculateCompletionIndexRequest(
+"speculative-completion-index-request",
+llvm::cl::desc(
+R"(If set to true and static index is enabled, this will send an
+asynchronous speculative index request, based on the index request for
+the last code completion on the same file and the filter text typed
+before the cursor, before sema code completion is invoked. This can
+reduce the code completion latency (by roughly latency of sema code
+completion) if the speculative reque

[PATCH] D50962: [clangd] Speculative code completion index request before Sema is run.

[Eric Liu via Phabricator via cfe-commits]

ioeric added inline comments.



Comment at: unittests/clangd/CodeCompleteTests.cpp:1710
+  $bol^
+  ab$ab^
+  x.ab$dot^

kadircet wrote:
> Maybe an EXPECT for this one as well?
Nice catch!


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D50962



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

[PATCH] D50926: [SourceManager] Extract 'findFileIDsForFile' from 'translateFile' to allow mapping from one file entry to multiple FileIDs

[Eric Liu via Phabricator via cfe-commits]

ioeric added inline comments.



Comment at: lib/Basic/SourceManager.cpp:1705
 
   // If we haven't found what we want yet, try again, but this time stat()
   // each of the files in case the files have changed since we originally

Do we also need this in `findFileIDsForFile`?



Comment at: unittests/Basic/SourceManagerTest.cpp:380
 
+TEST_F(SourceManagerTest, findFileIDsForFile) {
+  const char *header = "int x;";

Could you add a test case for getting file ID for main file, just to make sure 
we also covered cases handled by `if (MainFileID.isValid()) {...}`  code path 
in `translateFile()`?


Repository:
  rC Clang

https://reviews.llvm.org/D50926



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

[PATCH] D50502: [clangd] Initial cancellation mechanism for LSP requests.

[Eric Liu via Phabricator via cfe-commits]

ioeric added inline comments.



Comment at: clangd/Cancellation.h:96
+/// checks using it to avoid extra lookups in the Context.
+class CancellationToken {
+public:

ilya-biryukov wrote:
> ioeric wrote:
> > As chatted offline, I have questions about the motivation of the 
> > `CancellationToken` class. Intuitively, it seems to me that this can be 
> > merged into `TaskHandle`, and we can simply stash the `TaskHandle` instead 
> > of a token into the context. There would be fewer states to maintain, and 
> > the design would be a bit more straightforward. I might be missing 
> > context/concerns here, so I'd like to hear what you and Ilya think. 
> > @ilya-biryukov 
> I am for splitting cancellation checks from cancellation triggers.
> The processing code only needs to check if it was cancelled and exposing the 
> `cancel()` does not add any useful functionality, merely ways to misuse it.
Couldn't we prevent this by passing only const handle references to users who 
are not expect to call `cancel()`?


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D50502



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

[PATCH] D50897: [clangd] Allow using experimental Dex index

[Eric Liu via Phabricator via cfe-commits]

ioeric added inline comments.



Comment at: clang-tools-extra/clangd/index/MemIndex.h:50
 
+// FIXME(kbobyrev): Document this one.
+std::shared_ptr>

Please add documentation.



Comment at: clang-tools-extra/clangd/tool/ClangdMain.cpp:96
 
+namespace {
+

Why the move? A chunk of code in the middle of flags seems strange.


https://reviews.llvm.org/D50897



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

[PATCH] D50897: [clangd] Allow using experimental Dex index

[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


https://reviews.llvm.org/D50897



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

[PATCH] D50898: [clangd] Suggest code-completions for overriding base class virtual methods.

[Eric Liu via Phabricator via cfe-commits]

ioeric added a comment.

In https://reviews.llvm.org/D50898#1205756, @hokein wrote:

> I think it is reasonable to make Sema support suggesting override methods, 
> instead of implementing it in clangd side?


drive-by: +1 to this.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D50898



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

[PATCH] D50889: [clangd] Make FileIndex aware of the main 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.

lgtm


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D50889



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

[PATCH] D50926: [SourceManager] Extract 'findFileIDsForFile' from 'translateFile' to allow mapping from one file entry to multiple FileIDs

[Eric Liu via Phabricator via cfe-commits]

ioeric added inline comments.



Comment at: lib/Basic/SourceManager.cpp:1705
 
   // If we haven't found what we want yet, try again, but this time stat()
   // each of the files in case the files have changed since we originally

arphaman wrote:
> ioeric wrote:
> > Do we also need this in `findFileIDsForFile`?
> I don't really need this for my use case.
But it's not clear from the interface AFAICT. We should either handle this case 
(maybe controlled with a flag), or make it clear in the API (with a different 
name or documentation).


Repository:
  rC Clang

https://reviews.llvm.org/D50926



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

[PATCH] D51088: [clangd] Get rid of regexes in CanonicalIncludes

[Eric Liu via Phabricator via cfe-commits]

ioeric added inline comments.



Comment at: clangd/index/CanonicalIncludes.h:44
   /// Maps all files matching \p RE to \p CanonicalPath
-  void addRegexMapping(llvm::StringRef RE, llvm::StringRef CanonicalPath);
+  void addSuffixMapping(llvm::StringRef Suffix, llvm::StringRef CanonicalPath);
 

It seems that this is only file path suffix matching (by components) now. We 
should probably rename the function to be explicit.



Comment at: clangd/index/CanonicalIncludes.h:62
+  llvm::StringMap SuffixHeaderMapping;
+  /// Maximum number of path components stored in a key of SuffixHeaderMapping.
+  int MaxSuffixComponents = 0;

It might be worth documenting why this is useful (i.e. the optimization).


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D51088



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

[PATCH] D50962: [clangd] Speculative code completion index request before Sema is run.

[Eric Liu via Phabricator via cfe-commits]

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

- minor cleanup.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D50962

Files:
  clangd/ClangdServer.cpp
  clangd/ClangdServer.h
  clangd/CodeComplete.cpp
  clangd/CodeComplete.h
  clangd/index/Index.cpp
  clangd/index/Index.h
  clangd/tool/ClangdMain.cpp
  unittests/clangd/CodeCompleteTests.cpp
  unittests/clangd/IndexTests.cpp

Index: unittests/clangd/IndexTests.cpp
===
--- unittests/clangd/IndexTests.cpp
+++ unittests/clangd/IndexTests.cpp
@@ -321,6 +321,21 @@
   EXPECT_EQ(M.Name, "right");
 }
 
+TEST(AsyncFuzzyFind, Simple) {
+  MemIndex Idx;
+  Idx.build(generateSymbols({"ns::abc", "ns::xyz"}));
+
+  FuzzyFindRequest Req;
+  Req.Query = "";
+  Req.Scopes = {"ns::"};
+
+  AsyncFuzzyFind Async(Idx, Req);
+
+  EXPECT_EQ(Async.getRequest(), Req);
+  EXPECT_THAT(Async.getResult(),
+  UnorderedElementsAre(Named("abc"), Named("xyz")));
+}
+
 } // namespace
 } // namespace clangd
 } // namespace clang
Index: unittests/clangd/CodeCompleteTests.cpp
===
--- unittests/clangd/CodeCompleteTests.cpp
+++ unittests/clangd/CodeCompleteTests.cpp
@@ -923,7 +923,11 @@
llvm::function_ref
Callback) const override {}
 
-  const std::vector allRequests() const { return Requests; }
+  const std::vector consumeRequests() const {
+auto Reqs = std::move(Requests);
+Requests.clear();
+return Reqs;
+  }
 
 private:
   mutable std::vector Requests;
@@ -934,7 +938,7 @@
   IndexRequestCollector Requests;
   Opts.Index = &Requests;
   completions(Code, {}, Opts);
-  return Requests.allRequests();
+  return Requests.consumeRequests();
 }
 
 TEST(CompletionTest, UnqualifiedIdQuery) {
@@ -1700,6 +1704,75 @@
   }
 }
 
+TEST(SpeculateCompletionFilter, Filters) {
+  Annotations F(R"cpp($bof^
+  $bol^
+  ab$ab^
+  x.ab$dot^
+  x.$dotempty^
+  x::ab$scoped^
+  x::$scopedempty^
+
+  )cpp");
+  auto speculate = [&](StringRef PointName) {
+auto Filter = speculateCompletionFilter(F.code(), F.point(PointName));
+assert(Filter);
+return *Filter;
+  };
+  EXPECT_EQ(speculate("bof"), "");
+  EXPECT_EQ(speculate("bol"), "");
+  EXPECT_EQ(speculate("ab"), "ab");
+  EXPECT_EQ(speculate("dot"), "ab");
+  EXPECT_EQ(speculate("dotempty"), "");
+  EXPECT_EQ(speculate("scoped"), "ab");
+  EXPECT_EQ(speculate("scopedempty"), "");
+}
+
+TEST(CompletionTest, EnableSpeculativeIndexRequest) {
+  MockFSProvider FS;
+  MockCompilationDatabase CDB;
+  IgnoreDiagnostics DiagConsumer;
+  ClangdServer Server(CDB, FS, DiagConsumer, ClangdServer::optsForTest());
+
+  auto File = testPath("foo.cpp");
+  Annotations Test(R"cpp(
+  namespace ns1 { int abc; }
+  namespace ns2 { int abc; }
+  void f() { ns1::ab$1^; ns1::ab$2^; }
+  void f() { ns2::ab$3^; }
+  )cpp");
+  runAddDocument(Server, File, Test.code());
+  clangd::CodeCompleteOptions Opts = {};
+
+  IndexRequestCollector Requests;
+  Opts.Index = &Requests;
+  Opts.SpeculativeIndexRequest = true;
+
+  auto CompleteAtPoint = [&](StringRef P) {
+cantFail(runCodeComplete(Server, File, Test.point(P), Opts));
+// Sleep for a while to make sure asynchronous call (if applicable) is also
+// triggered before callback is invoked.
+std::this_thread::sleep_for(std::chrono::milliseconds(100));
+  };
+
+  CompleteAtPoint("1");
+  auto Reqs1 = Requests.consumeRequests();
+  ASSERT_EQ(Reqs1.size(), 1u);
+  EXPECT_THAT(Reqs1[0].Scopes, UnorderedElementsAre("ns1::"));
+
+  CompleteAtPoint("2");
+  auto Reqs2 = Requests.consumeRequests();
+  // Speculation succeeded. Used speculative index result.
+  ASSERT_EQ(Reqs2.size(), 1u);
+  EXPECT_EQ(Reqs2[0], Reqs1[0]);
+
+  CompleteAtPoint("3");
+  // Speculation failed. Sent speculative index request and the new index
+  // request after sema.
+  auto Reqs3 = Requests.consumeRequests();
+  ASSERT_EQ(Reqs3.size(), 2u);
+}
+
 } // namespace
 } // namespace clangd
 } // namespace clang
Index: clangd/tool/ClangdMain.cpp
===
--- clangd/tool/ClangdMain.cpp
+++ clangd/tool/ClangdMain.cpp
@@ -164,6 +164,20 @@
"an include line will be inserted or not."),
 llvm::cl::init(true));
 
+static llvm::cl::opt SpeculateCompletionIndexRequest(
+"speculative-completion-index-request",
+llvm::cl::desc(
+R"(If set to true and static index is enabled, this will send an
+asynchronous speculative index request, based on the index request for
+the last code completion on the same file and the filter text typed
+before the cursor, before sema code completion is invoked. This can
+reduce the code completion latency (by roughly latency of sema code
+completion) if the speculative request is the same as the one generated
+

[PATCH] D51088: [clangd] Get rid of regexes in CanonicalIncludes

[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:24
+  int Components = 0;
+  for (auto It = llvm::sys::path::begin(Suffix),
+End = llvm::sys::path::end(Suffix);

Would `int Components = begin(Suffix)  - end(Suffix);` work here?



Comment at: clangd/index/CanonicalIncludes.cpp:79
+   It != End; ++It) {
+++Components;
+if (MaxSuffixComponents < Components)

Maybe merge `Components` and break condition into `for loop` condition?


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D51088



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

[PATCH] D51155: [clangd] Allow to merge symbols on-the-fly in global-symbol-builder

[Eric Liu via Phabricator via cfe-commits]

ioeric added a comment.

Overall looks good to me.




Comment at: clangd/global-symbol-builder/GlobalSymbolBuilderMain.cpp:61
+/// rely on MR use-case to work properly.
+llvm::cl::init(false));
+

AFAIK, this flag would basically depend on the executor being used. If executor 
can provide information like whether all tasks share memory, we can make the 
decision totally based on the selected executor (i.e. one fewer option).


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D51155



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

[PATCH] D51155: [clangd] Allow to merge symbols on-the-fly in global-symbol-builder

[Eric Liu via Phabricator via cfe-commits]

ioeric added inline comments.



Comment at: clangd/global-symbol-builder/GlobalSymbolBuilderMain.cpp:61
+/// rely on MR use-case to work properly.
+llvm::cl::init(false));
+

ilya-biryukov wrote:
> ioeric wrote:
> > AFAIK, this flag would basically depend on the executor being used. If 
> > executor can provide information like whether all tasks share memory, we 
> > can make the decision totally based on the selected executor (i.e. one 
> > fewer option).
> That SG. So we could add a virtual method to executor that would provide us 
> with this information and then we can get rid of that option, does that LG?
> However, do we have non-single-binary executor implementations upstream? We 
> need to leave a way to run the code that actually uses YAML serialization to 
> make sure we can experiment when things break.
Sounds good.

> However, do we have non-single-binary executor implementations upstream? We 
> need to leave a way to run the code that actually uses YAML serialization to 
> make sure we can experiment when things break.
The framework allows this, so I'm not sure if there is other downstream 
implementations that do this (probably not?). But I don't think we can get rid 
of the YAML serialization at this point because we still dump everything to 
yaml in the end? 


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D51155



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

[PATCH] D46000: [AST] Added a helper to extract a user-friendly text of a comment.

[Eric Liu via Phabricator via cfe-commits]

ioeric added a comment.

Thanks for adding the tests!




Comment at: include/clang/AST/RawCommentList.h:138
+  /// the overload with ASTContext in the rest of the code.
+  std::string getFormattedText(const SourceManager &SourceMgr,
+   DiagnosticsEngine &Diags) const;

I think we can get rid of the interface that takes `ASTContext`? If 
`SourceManager` and `Diags` are sufficient, I don't see why we would want 
another interface for ASTContext.



Comment at: lib/AST/RawCommentList.cpp:352
+  // comments::Lexer. Therefore, we just use default-constructed options.
+  CommentOptions DefOpts;
+  comments::CommandTraits EmptyTraits(Allocator, DefOpts);

I'm not quite sure about this. Could we just require a `CommandTraits` in the 
interface? And only make this assumption in tests?



Comment at: unittests/AST/CommentTextTest.cpp:32
+  std::string formatComment(llvm::StringRef CommentText) {
+llvm::IntrusiveRefCntPtr EmptyFS(
+new vfs::InMemoryFileSystem);

`SourceManagerForFile` added in D46176 should save you a few lines here. (I'm 
landing it right now...)


Repository:
  rC Clang

https://reviews.llvm.org/D46000



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

[PATCH] D46176: Add SourceManagerForFile helper which sets up SourceManager and dependencies for a single file with code snippet

[Eric Liu via Phabricator via cfe-commits]

ioeric updated this revision to Diff 146004.
ioeric marked 4 inline comments as done.
ioeric added a comment.

- address review comments.


Repository:
  rC Clang

https://reviews.llvm.org/D46176

Files:
  include/clang/Basic/SourceManager.h
  lib/Basic/SourceManager.cpp
  lib/Format/Format.cpp
  lib/Format/SortJavaScriptImports.cpp
  lib/Format/TokenAnalyzer.cpp
  lib/Format/TokenAnalyzer.h

Index: lib/Format/TokenAnalyzer.h
===
--- lib/Format/TokenAnalyzer.h
+++ lib/Format/TokenAnalyzer.h
@@ -37,44 +37,24 @@
 class Environment {
 public:
   Environment(SourceManager &SM, FileID ID, ArrayRef Ranges)
-  : ID(ID), CharRanges(Ranges.begin(), Ranges.end()), SM(SM),
-  FirstStartColumn(0),
-  NextStartColumn(0),
-  LastStartColumn(0) {}
-
-  Environment(FileID ID, std::unique_ptr FileMgr,
-  std::unique_ptr VirtualSM,
-  std::unique_ptr Diagnostics,
-  const std::vector &CharRanges,
-  unsigned FirstStartColumn,
-  unsigned NextStartColumn,
-  unsigned LastStartColumn)
-  : ID(ID), CharRanges(CharRanges.begin(), CharRanges.end()),
-SM(*VirtualSM), 
-FirstStartColumn(FirstStartColumn),
-NextStartColumn(NextStartColumn),
-LastStartColumn(LastStartColumn),
-FileMgr(std::move(FileMgr)),
-VirtualSM(std::move(VirtualSM)), Diagnostics(std::move(Diagnostics)) {}
+  : SM(SM), ID(ID), CharRanges(Ranges.begin(), Ranges.end()),
+FirstStartColumn(0), NextStartColumn(0), LastStartColumn(0) {}
 
   // This sets up an virtual file system with file \p FileName containing the
   // fragment \p Code. Assumes that \p Code starts at \p FirstStartColumn,
   // that the next lines of \p Code should start at \p NextStartColumn, and
   // that \p Code should end at \p LastStartColumn if it ends in newline.
   // See also the documentation of clang::format::internal::reformat.
-  static std::unique_ptr
-  CreateVirtualEnvironment(StringRef Code, StringRef FileName,
-   ArrayRef Ranges,
-   unsigned FirstStartColumn = 0,
-   unsigned NextStartColumn = 0,
-   unsigned LastStartColumn = 0);
+  Environment(StringRef Code, StringRef FileName,
+  ArrayRef Ranges, unsigned FirstStartColumn = 0,
+  unsigned NextStartColumn = 0, unsigned LastStartColumn = 0);
 
   FileID getFileID() const { return ID; }
 
-  ArrayRef getCharRanges() const { return CharRanges; }
-
   const SourceManager &getSourceManager() const { return SM; }
 
+  ArrayRef getCharRanges() const { return CharRanges; }
+
   // Returns the column at which the fragment of code managed by this
   // environment starts.
   unsigned getFirstStartColumn() const { return FirstStartColumn; }
@@ -88,19 +68,18 @@
   unsigned getLastStartColumn() const { return LastStartColumn; }
 
 private:
+  // This is only set if constructed from string.
+  std::unique_ptr VirtualSM;
+
+  // This refers to either a SourceManager provided by users or VirtualSM
+  // created for a single file.
+  SourceManager &SM;
   FileID ID;
+
   SmallVector CharRanges;
-  SourceManager &SM;
   unsigned FirstStartColumn;
   unsigned NextStartColumn;
   unsigned LastStartColumn;
-
-  // The order of these fields are important - they should be in the same order
-  // as they are created in `CreateVirtualEnvironment` so that they can be
-  // deleted in the reverse order as they are created.
-  std::unique_ptr FileMgr;
-  std::unique_ptr VirtualSM;
-  std::unique_ptr Diagnostics;
 };
 
 class TokenAnalyzer : public UnwrappedLineConsumer {
Index: lib/Format/TokenAnalyzer.cpp
===
--- lib/Format/TokenAnalyzer.cpp
+++ lib/Format/TokenAnalyzer.cpp
@@ -34,48 +34,19 @@
 namespace clang {
 namespace format {
 
-// This sets up an virtual file system with file \p FileName containing \p
-// Code.
-std::unique_ptr
-Environment::CreateVirtualEnvironment(StringRef Code, StringRef FileName,
-  ArrayRef Ranges,
-  unsigned FirstStartColumn,
-  unsigned NextStartColumn,
-  unsigned LastStartColumn) {
-  // This is referenced by `FileMgr` and will be released by `FileMgr` when it
-  // is deleted.
-  IntrusiveRefCntPtr InMemoryFileSystem(
-  new vfs::InMemoryFileSystem);
-  // This is passed to `SM` as reference, so the pointer has to be referenced
-  // in `Environment` so that `FileMgr` can out-live this function scope.
-  std::unique_ptr FileMgr(
-  new FileManager(FileSystemOptions(), InMemoryFileSystem));
-  // This is passed to `SM` as reference, so the pointer has to be referenced
-  // by `Environment` due to the same reason above.
-  std::unique_ptr Diagnostics(new DiagnosticsEngine(
-  

[PATCH] D46176: Add SourceManagerForFile helper which sets up SourceManager and dependencies for a single file with code snippet

[Eric Liu via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL331923: Add SourceManagerForFile helper which sets up 
SourceManager and dependencies… (authored by ioeric, committed by ).
Herald added a subscriber: llvm-commits.

Repository:
  rL LLVM

https://reviews.llvm.org/D46176

Files:
  cfe/trunk/include/clang/Basic/SourceManager.h
  cfe/trunk/lib/Basic/SourceManager.cpp
  cfe/trunk/lib/Format/Format.cpp
  cfe/trunk/lib/Format/SortJavaScriptImports.cpp
  cfe/trunk/lib/Format/TokenAnalyzer.cpp
  cfe/trunk/lib/Format/TokenAnalyzer.h

Index: cfe/trunk/lib/Format/SortJavaScriptImports.cpp
===
--- cfe/trunk/lib/Format/SortJavaScriptImports.cpp
+++ cfe/trunk/lib/Format/SortJavaScriptImports.cpp
@@ -445,10 +445,9 @@
 ArrayRef Ranges,
 StringRef FileName) {
   // FIXME: Cursor support.
-  std::unique_ptr Env =
-  Environment::CreateVirtualEnvironment(Code, FileName, Ranges);
-  JavaScriptImportSorter Sorter(*Env, Style);
-  return Sorter.process().first;
+  return JavaScriptImportSorter(Environment(Code, FileName, Ranges), Style)
+  .process()
+  .first;
 }
 
 } // end namespace format
Index: cfe/trunk/lib/Format/TokenAnalyzer.h
===
--- cfe/trunk/lib/Format/TokenAnalyzer.h
+++ cfe/trunk/lib/Format/TokenAnalyzer.h
@@ -37,44 +37,24 @@
 class Environment {
 public:
   Environment(SourceManager &SM, FileID ID, ArrayRef Ranges)
-  : ID(ID), CharRanges(Ranges.begin(), Ranges.end()), SM(SM),
-  FirstStartColumn(0),
-  NextStartColumn(0),
-  LastStartColumn(0) {}
-
-  Environment(FileID ID, std::unique_ptr FileMgr,
-  std::unique_ptr VirtualSM,
-  std::unique_ptr Diagnostics,
-  const std::vector &CharRanges,
-  unsigned FirstStartColumn,
-  unsigned NextStartColumn,
-  unsigned LastStartColumn)
-  : ID(ID), CharRanges(CharRanges.begin(), CharRanges.end()),
-SM(*VirtualSM), 
-FirstStartColumn(FirstStartColumn),
-NextStartColumn(NextStartColumn),
-LastStartColumn(LastStartColumn),
-FileMgr(std::move(FileMgr)),
-VirtualSM(std::move(VirtualSM)), Diagnostics(std::move(Diagnostics)) {}
+  : SM(SM), ID(ID), CharRanges(Ranges.begin(), Ranges.end()),
+FirstStartColumn(0), NextStartColumn(0), LastStartColumn(0) {}
 
   // This sets up an virtual file system with file \p FileName containing the
   // fragment \p Code. Assumes that \p Code starts at \p FirstStartColumn,
   // that the next lines of \p Code should start at \p NextStartColumn, and
   // that \p Code should end at \p LastStartColumn if it ends in newline.
   // See also the documentation of clang::format::internal::reformat.
-  static std::unique_ptr
-  CreateVirtualEnvironment(StringRef Code, StringRef FileName,
-   ArrayRef Ranges,
-   unsigned FirstStartColumn = 0,
-   unsigned NextStartColumn = 0,
-   unsigned LastStartColumn = 0);
+  Environment(StringRef Code, StringRef FileName,
+  ArrayRef Ranges, unsigned FirstStartColumn = 0,
+  unsigned NextStartColumn = 0, unsigned LastStartColumn = 0);
 
   FileID getFileID() const { return ID; }
 
-  ArrayRef getCharRanges() const { return CharRanges; }
-
   const SourceManager &getSourceManager() const { return SM; }
 
+  ArrayRef getCharRanges() const { return CharRanges; }
+
   // Returns the column at which the fragment of code managed by this
   // environment starts.
   unsigned getFirstStartColumn() const { return FirstStartColumn; }
@@ -88,19 +68,18 @@
   unsigned getLastStartColumn() const { return LastStartColumn; }
 
 private:
+  // This is only set if constructed from string.
+  std::unique_ptr VirtualSM;
+
+  // This refers to either a SourceManager provided by users or VirtualSM
+  // created for a single file.
+  SourceManager &SM;
   FileID ID;
+
   SmallVector CharRanges;
-  SourceManager &SM;
   unsigned FirstStartColumn;
   unsigned NextStartColumn;
   unsigned LastStartColumn;
-
-  // The order of these fields are important - they should be in the same order
-  // as they are created in `CreateVirtualEnvironment` so that they can be
-  // deleted in the reverse order as they are created.
-  std::unique_ptr FileMgr;
-  std::unique_ptr VirtualSM;
-  std::unique_ptr Diagnostics;
 };
 
 class TokenAnalyzer : public UnwrappedLineConsumer {
Index: cfe/trunk/lib/Format/Format.cpp
===
--- cfe/trunk/lib/Format/Format.cpp
+++ cfe/trunk/lib/Format/Format.cpp
@@ -1905,10 +1905,9 @@
 StringRef FileName, StringRef Code, const FormatStyle &Style,
 llvm::function_ref
 GetOffsetAfterSequence) {
-  std::unique_ptr E

[PATCH] D46496: [Tooling] Pull #include manipulation code from clangFormat into libToolingCore.

[Eric Liu via Phabricator via cfe-commits]

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

- Merged with origin/master


Repository:
  rC Clang

https://reviews.llvm.org/D46496

Files:
  include/clang/Format/Format.h
  include/clang/Tooling/Core/HeaderIncludes.h
  lib/Format/Format.cpp
  lib/Tooling/Core/CMakeLists.txt
  lib/Tooling/Core/HeaderIncludes.cpp
  unittests/Format/CleanupTest.cpp
  unittests/Format/FormatTest.cpp
  unittests/Format/SortIncludesTest.cpp
  unittests/Tooling/CMakeLists.txt
  unittests/Tooling/HeaderIncludesTest.cpp

Index: unittests/Tooling/HeaderIncludesTest.cpp
===
--- /dev/null
+++ unittests/Tooling/HeaderIncludesTest.cpp
@@ -0,0 +1,527 @@
+//===- unittest/Tooling/CleanupTest.cpp - Include insertion/deletion tests ===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+
+#include "clang/Tooling/Core/HeaderIncludes.h"
+#include "../Tooling/ReplacementTest.h"
+#include "../Tooling/RewriterTestContext.h"
+#include "clang/Format/Format.h"
+#include "clang/Tooling/Core/Replacement.h"
+
+#include "gtest/gtest.h"
+
+using clang::tooling::ReplacementTest;
+using clang::tooling::toReplacements;
+
+namespace clang {
+namespace tooling {
+namespace {
+
+class HeaderIncludesTest : public ::testing::Test {
+protected:
+  std::string insert(llvm::StringRef Code, llvm::StringRef Header) {
+HeaderIncludes Includes(FileName, Code, Style);
+assert(Header.startswith("\"") || Header.startswith("<"));
+auto R = Includes.insert(Header.trim("\"<>"), Header.startswith("<"));
+if (!R)
+  return Code;
+auto Result = applyAllReplacements(Code, Replacements(*R));
+EXPECT_TRUE(static_cast(Result));
+return *Result;
+  }
+
+  std::string remove(llvm::StringRef Code, llvm::StringRef Header) {
+HeaderIncludes Includes(FileName, Code, Style);
+assert(Header.startswith("\"") || Header.startswith("<"));
+auto Replaces = Includes.remove(Header.trim("\"<>"), Header.startswith("<"));
+auto Result = applyAllReplacements(Code, Replaces);
+EXPECT_TRUE(static_cast(Result));
+return *Result;
+  }
+
+  const std::string FileName = "fix.cpp";
+  IncludeStyle Style = format::getLLVMStyle().IncludeStyle;
+};
+
+TEST_F(HeaderIncludesTest, NoExistingIncludeWithoutDefine) {
+  std::string Code = "int main() {}";
+  std::string Expected = "#include \"a.h\"\n"
+ "int main() {}";
+  EXPECT_EQ(Expected, insert(Code, "\"a.h\""));
+}
+
+TEST_F(HeaderIncludesTest, NoExistingIncludeWithDefine) {
+  std::string Code = "#ifndef A_H\n"
+ "#define A_H\n"
+ "class A {};\n"
+ "#define MMM 123\n"
+ "#endif";
+  std::string Expected = "#ifndef A_H\n"
+ "#define A_H\n"
+ "#include \"b.h\"\n"
+ "class A {};\n"
+ "#define MMM 123\n"
+ "#endif";
+
+  EXPECT_EQ(Expected, insert(Code, "\"b.h\""));
+}
+
+TEST_F(HeaderIncludesTest, InsertBeforeCategoryWithLowerPriority) {
+  std::string Code = "#ifndef A_H\n"
+ "#define A_H\n"
+ "\n"
+ "\n"
+ "\n"
+ "#include \n"
+ "class A {};\n"
+ "#define MMM 123\n"
+ "#endif";
+  std::string Expected = "#ifndef A_H\n"
+ "#define A_H\n"
+ "\n"
+ "\n"
+ "\n"
+ "#include \"a.h\"\n"
+ "#include \n"
+ "class A {};\n"
+ "#define MMM 123\n"
+ "#endif";
+
+  EXPECT_EQ(Expected, insert(Code, "\"a.h\""));
+}
+
+TEST_F(HeaderIncludesTest, InsertAfterMainHeader) {
+  std::string Code = "#include \"fix.h\"\n"
+ "\n"
+ "int main() {}";
+  std::string Expected = "#include \"fix.h\"\n"
+ "#include \n"
+ "\n"
+ "int main() {}";
+  Style = format::getGoogleStyle(format::FormatStyle::LanguageKind::LK_Cpp)
+  .IncludeStyle;
+  EXPECT_EQ(Expected, insert(Code, ""));
+}
+
+TEST_F(HeaderIncludesTest, InsertBeforeSystemHeaderLLVM) {
+  std::string Code = "#include \n"
+ "\n"
+ "int main() {}";
+  std::string Expected = "#include \"z.h\"\n"
+ "#include \n"
+ "\n"
+ "int main() {}";
+  EXPECT_EQ(Expected, insert(Code, "\"z.h\""));
+}
+
+TEST_F(HeaderIncludesTest, InsertAfterSystemHeaderGoogle) {
+  std::strin

[PATCH] D46496: [Tooling] Pull #include manipulation code from clangFormat into libToolingCore.

[Eric Liu via Phabricator via cfe-commits]

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

- Merged with origin/master


Repository:
  rC Clang

https://reviews.llvm.org/D46496

Files:
  include/clang/Format/Format.h
  include/clang/Tooling/Core/HeaderIncludes.h
  lib/Format/Format.cpp
  lib/Tooling/Core/CMakeLists.txt
  lib/Tooling/Core/HeaderIncludes.cpp
  unittests/Format/CleanupTest.cpp
  unittests/Format/FormatTest.cpp
  unittests/Format/SortIncludesTest.cpp
  unittests/Tooling/CMakeLists.txt
  unittests/Tooling/HeaderIncludesTest.cpp

Index: unittests/Tooling/HeaderIncludesTest.cpp
===
--- /dev/null
+++ unittests/Tooling/HeaderIncludesTest.cpp
@@ -0,0 +1,527 @@
+//===- unittest/Tooling/CleanupTest.cpp - Include insertion/deletion tests ===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+
+#include "clang/Tooling/Core/HeaderIncludes.h"
+#include "../Tooling/ReplacementTest.h"
+#include "../Tooling/RewriterTestContext.h"
+#include "clang/Format/Format.h"
+#include "clang/Tooling/Core/Replacement.h"
+
+#include "gtest/gtest.h"
+
+using clang::tooling::ReplacementTest;
+using clang::tooling::toReplacements;
+
+namespace clang {
+namespace tooling {
+namespace {
+
+class HeaderIncludesTest : public ::testing::Test {
+protected:
+  std::string insert(llvm::StringRef Code, llvm::StringRef Header) {
+HeaderIncludes Includes(FileName, Code, Style);
+assert(Header.startswith("\"") || Header.startswith("<"));
+auto R = Includes.insert(Header.trim("\"<>"), Header.startswith("<"));
+if (!R)
+  return Code;
+auto Result = applyAllReplacements(Code, Replacements(*R));
+EXPECT_TRUE(static_cast(Result));
+return *Result;
+  }
+
+  std::string remove(llvm::StringRef Code, llvm::StringRef Header) {
+HeaderIncludes Includes(FileName, Code, Style);
+assert(Header.startswith("\"") || Header.startswith("<"));
+auto Replaces = Includes.remove(Header.trim("\"<>"), Header.startswith("<"));
+auto Result = applyAllReplacements(Code, Replaces);
+EXPECT_TRUE(static_cast(Result));
+return *Result;
+  }
+
+  const std::string FileName = "fix.cpp";
+  IncludeStyle Style = format::getLLVMStyle().IncludeStyle;
+};
+
+TEST_F(HeaderIncludesTest, NoExistingIncludeWithoutDefine) {
+  std::string Code = "int main() {}";
+  std::string Expected = "#include \"a.h\"\n"
+ "int main() {}";
+  EXPECT_EQ(Expected, insert(Code, "\"a.h\""));
+}
+
+TEST_F(HeaderIncludesTest, NoExistingIncludeWithDefine) {
+  std::string Code = "#ifndef A_H\n"
+ "#define A_H\n"
+ "class A {};\n"
+ "#define MMM 123\n"
+ "#endif";
+  std::string Expected = "#ifndef A_H\n"
+ "#define A_H\n"
+ "#include \"b.h\"\n"
+ "class A {};\n"
+ "#define MMM 123\n"
+ "#endif";
+
+  EXPECT_EQ(Expected, insert(Code, "\"b.h\""));
+}
+
+TEST_F(HeaderIncludesTest, InsertBeforeCategoryWithLowerPriority) {
+  std::string Code = "#ifndef A_H\n"
+ "#define A_H\n"
+ "\n"
+ "\n"
+ "\n"
+ "#include \n"
+ "class A {};\n"
+ "#define MMM 123\n"
+ "#endif";
+  std::string Expected = "#ifndef A_H\n"
+ "#define A_H\n"
+ "\n"
+ "\n"
+ "\n"
+ "#include \"a.h\"\n"
+ "#include \n"
+ "class A {};\n"
+ "#define MMM 123\n"
+ "#endif";
+
+  EXPECT_EQ(Expected, insert(Code, "\"a.h\""));
+}
+
+TEST_F(HeaderIncludesTest, InsertAfterMainHeader) {
+  std::string Code = "#include \"fix.h\"\n"
+ "\n"
+ "int main() {}";
+  std::string Expected = "#include \"fix.h\"\n"
+ "#include \n"
+ "\n"
+ "int main() {}";
+  Style = format::getGoogleStyle(format::FormatStyle::LanguageKind::LK_Cpp)
+  .IncludeStyle;
+  EXPECT_EQ(Expected, insert(Code, ""));
+}
+
+TEST_F(HeaderIncludesTest, InsertBeforeSystemHeaderLLVM) {
+  std::string Code = "#include \n"
+ "\n"
+ "int main() {}";
+  std::string Expected = "#include \"z.h\"\n"
+ "#include \n"
+ "\n"
+ "int main() {}";
+  EXPECT_EQ(Expected, insert(Code, "\"z.h\""));
+}
+
+TEST_F(HeaderIncludesTest, InsertAfterSystemHeaderGoogle) {
+  std::strin

[PATCH] D46497: [clangd] Populate #include insertions as additional edits in completion items.

[Eric Liu via Phabricator via cfe-commits]

ioeric added a comment.

In https://reviews.llvm.org/D46497#1089755, @sammccall wrote:

> I'm concerned about the scope of this patch - it does too many things and 
> touches too many files for me to feel comfortable that I understand it well 
> enough to review.
>  Is it possible to split it up? (You mention 6 distinct things in the 
> description).


Sorry about the big change! And thanks for the early feedback on headers 
library!

I think there are a few things going on here:

1. Removing the existing way of include insertion (additional command)
2. Populating #include insertion in code completion workflow.
3. Refactoring #include path calculation to make 2 work
4. Move some helpers (e.g. replacementToEdit) to public header.
5. Share InclusionLocation between preamble code and header insertion code.

I was trying to keep 1 and 2 in the same patch so that we don't regress 
features, but it's probably fine if no one is actually using it... I'm happy to 
split that out if having them in the same patch makes review hard.

I think 2 and 3 could stay in the same patch as it would be hard to see where 
the design in 3 came from without 2. But if you got the idea, I guess they 
could still be split.

4 and 5 should be independent enough to be 2 separate patches. I thought the 
changes were small and could be included in this patch. I'll split them as well 
now that you mention.




Comment at: clangd/Headers.h:1
 //===--- Headers.h - Include headers -*- 
C++-*-===//
 //

sammccall wrote:
> The interface to this header looks a bit complex and entangled after this 
> change.
> - The construction/lifecycle of IncludePaths is confusing (comments below)
> - here we provide all the information needed to compute the edits (style, 
> code...) but don't actually do so
> 
> it would be nice to achieve a simpler interface with the restricted set of 
> functionality, or expand the functionality...
Refactored code according to your suggestions. PTAL

> here we provide all the information needed to compute the edits (style, 
> code...) but don't actually do so
Sorry... forgot to delete some leftover after an earlier revision. 



Comment at: clangd/Headers.h:29
 
+// A header inclusion in the main file.
+struct InclusionLocation {

sammccall wrote:
> I had some trouble working out the role of this struct (e.g. that it might 
> have been a *proposed* inclusion).
> Strictly it's not just the location, but also identifies the header being 
> included.
> 
> So this could maybe be
> ```
> // An #include directive that we found in the main file.
> struct Inclusion
> ```
> 
> but it does also seem a little strange that this is part of the public 
> interface at all (other than for testing)
Thanks! `Inclusion` sounds better.

Apart from header insertion and testing, this is also shared with the preamble 
code.



Comment at: clangd/Headers.h:57
+/// preprocessor and build configs.
+class IncludePaths {
+public:

sammccall wrote:
> This name suggests a set or sequence of include path entries (`-I` flags).
> Can we merge this with `IncludeInsertion` and call it `IncludeInserter` or 
> similar?
I think there are three pretty independent parts of include insertion:
1. URI/Verbatim to `HeaderFile` conversion (which lives in HeaderInsertion in 
CodeComplete.cpp right now).
2. Calculate include paths based on existing inclusions and HeaderSearch.
3. Calculate include insertion edits using `tooling::HeaderIncludes`.

Initially, I grouped all these in one class (something like `HeaderInsertion`) 
which returns an edit given the URIs in the symbol info, but it turned out to 
be hard to test. For example, among all 3 components, 2 was the most 
interesting piece to test, while an integration test is probably enough for 1 
and 3, but we would need to create URIs (due to 1) and retrieve include name 
from edits (due to 3) in order to test 2. Since each individual piece seemed 
independent enough, I decided to keep them separated. I also kept URI logic out 
of the header library so that it could potentially be shared with other 
features that are not index-based include insertion, or be moved out of clangd 
as a tooling library. 

In the new revision, I refactored the code according to your suggestions and 
got rid of this class.



Comment at: clangd/Headers.h:60
+  /// This must be called before preprocessor is run (this registers a
+  /// PPCallback on \p PP).
+  /// \p SM is used in the PPCallbacks to filter source locations in main file.

sammccall wrote:
> hmm, if we require the InclusionLocations to be prebuilt for the preamble, 
> why can't we do the same for non-preamble includes? i.e. can't we split up 
> the "collect existing includes" and "calculate new includes" parts to 
> simplify the interface here? Among other things, testing would be a ton 
> easier.
Good idea! 


==

[PATCH] D46497: [clangd] Populate #include insertions as additional edits in completion items.

[Eric Liu via Phabricator via cfe-commits]

ioeric updated this revision to Diff 146045.
ioeric marked 15 inline comments as done.
ioeric added a comment.

- Merged with origin/master
- Address review comments.
- Revert unintended change.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D46497

Files:
  clangd/ClangdLSPServer.cpp
  clangd/ClangdServer.cpp
  clangd/ClangdServer.h
  clangd/ClangdUnit.cpp
  clangd/ClangdUnit.h
  clangd/CodeComplete.cpp
  clangd/CodeComplete.h
  clangd/Headers.cpp
  clangd/Headers.h
  clangd/Protocol.cpp
  clangd/Protocol.h
  clangd/SourceCode.cpp
  clangd/SourceCode.h
  clangd/XRefs.cpp
  test/clangd/initialize-params-invalid.test
  test/clangd/initialize-params.test
  test/clangd/insert-include.test
  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
@@ -8,38 +8,96 @@
 //===--===//
 
 #include "Headers.h"
+
+#include "Compiler.h"
 #include "TestFS.h"
+#include "clang/Frontend/CompilerInstance.h"
+#include "clang/Frontend/CompilerInvocation.h"
+#include "clang/Frontend/FrontendActions.h"
+#include "clang/Lex/PreprocessorOptions.h"
 #include "gmock/gmock.h"
 #include "gtest/gtest.h"
 
 namespace clang {
 namespace clangd {
 namespace {
 
+using ::testing::AllOf;
+using ::testing::UnorderedElementsAre;
+
 class HeadersTest : public ::testing::Test {
 public:
   HeadersTest() {
 CDB.ExtraClangFlags = {SearchDirArg.c_str()};
 FS.Files[MainFile] = "";
+// Make sure directory sub/ exists.
+FS.Files[testPath("sub/EMPTY")] = "";
+  }
+
+private:
+  std::unique_ptr setupClang() {
+auto Cmd = CDB.getCompileCommand(MainFile);
+assert(static_cast(Cmd));
+auto VFS = FS.getFileSystem();
+VFS->setCurrentWorkingDirectory(Cmd->Directory);
+
+std::vector Argv;
+for (const auto &S : Cmd->CommandLine)
+  Argv.push_back(S.c_str());
+auto CI = clang::createInvocationFromCommandLine(
+Argv,
+CompilerInstance::createDiagnostics(new DiagnosticOptions(),
+&IgnoreDiags, false),
+VFS);
+EXPECT_TRUE(static_cast(CI));
+CI->getFrontendOpts().DisableFree = false;
+
+// The diagnostic options must be set before creating a CompilerInstance.
+CI->getDiagnosticOpts().IgnoreWarnings = true;
+auto Clang = prepareCompilerInstance(
+std::move(CI), /*Preamble=*/nullptr,
+llvm::MemoryBuffer::getMemBuffer(FS.Files[MainFile], MainFile),
+std::make_shared(), VFS, IgnoreDiags);
+
+EXPECT_FALSE(Clang->getFrontendOpts().Inputs.empty());
+return Clang;
   }
 
 protected:
+  std::vector collectIncludes() {
+auto Clang = setupClang();
+PreprocessOnlyAction Action;
+EXPECT_TRUE(
+Action.BeginSourceFile(*Clang, Clang->getFrontendOpts().Inputs[0]));
+std::vector Inclusions;
+Clang->getPreprocessor().addPPCallbacks(collectInclusionsInMainFileCallback(
+Clang->getSourceManager(), Inclusions));
+EXPECT_TRUE(Action.Execute());
+Action.EndSourceFile();
+return Inclusions;
+  }
+
   // Calculates the include path, or returns "" on error.
   std::string calculate(PathRef Original, PathRef Preferred = "",
+const std::vector &Inclusions = {},
 bool ExpectError = false) {
+auto Clang = setupClang();
+PreprocessOnlyAction Action;
+EXPECT_TRUE(
+Action.BeginSourceFile(*Clang, Clang->getFrontendOpts().Inputs[0]));
+
 if (Preferred.empty())
   Preferred = Original;
-auto VFS = FS.getFileSystem();
-auto Cmd = CDB.getCompileCommand(MainFile);
-assert(static_cast(Cmd));
-VFS->setCurrentWorkingDirectory(Cmd->Directory);
 auto ToHeaderFile = [](llvm::StringRef Header) {
   return HeaderFile{Header,
 /*Verbatim=*/!llvm::sys::path::is_absolute(Header)};
 };
-auto Path = calculateIncludePath(MainFile, FS.Files[MainFile],
- ToHeaderFile(Original),
- ToHeaderFile(Preferred), *Cmd, VFS);
+
+auto Path = calculateIncludePath(
+MainFile, CDB.getCompileCommand(MainFile)->Directory,
+Clang->getPreprocessor().getHeaderSearchInfo(), Inclusions,
+ToHeaderFile(Original), ToHeaderFile(Preferred));
+Action.EndSourceFile();
 if (!Path) {
   llvm::consumeError(Path.takeError());
   EXPECT_TRUE(ExpectError);
@@ -49,52 +107,31 @@
 }
 return std::move(*Path);
   }
+
   MockFSProvider FS;
   MockCompilationDatabase CDB;
   std::string MainFile = testPath("main.cpp");
   std::string Subdir = testPath("sub");
   std::string SearchDirArg = (llvm::Twine("-I") + Subdir).str();
+  IgnoringDiagConsumer IgnoreDiags;
 };
 
-TEST_F(Head

[PATCH] D46497: [clangd] Populate #include insertions as additional edits in completion items.

[Eric Liu via Phabricator via cfe-commits]

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

- Minor cleanup


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D46497

Files:
  clangd/ClangdLSPServer.cpp
  clangd/ClangdServer.cpp
  clangd/ClangdServer.h
  clangd/ClangdUnit.cpp
  clangd/ClangdUnit.h
  clangd/CodeComplete.cpp
  clangd/CodeComplete.h
  clangd/Headers.cpp
  clangd/Headers.h
  clangd/Protocol.cpp
  clangd/Protocol.h
  clangd/SourceCode.cpp
  clangd/SourceCode.h
  clangd/XRefs.cpp
  test/clangd/initialize-params-invalid.test
  test/clangd/initialize-params.test
  test/clangd/insert-include.test
  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
@@ -8,38 +8,96 @@
 //===--===//
 
 #include "Headers.h"
+
+#include "Compiler.h"
 #include "TestFS.h"
+#include "clang/Frontend/CompilerInstance.h"
+#include "clang/Frontend/CompilerInvocation.h"
+#include "clang/Frontend/FrontendActions.h"
+#include "clang/Lex/PreprocessorOptions.h"
 #include "gmock/gmock.h"
 #include "gtest/gtest.h"
 
 namespace clang {
 namespace clangd {
 namespace {
 
+using ::testing::AllOf;
+using ::testing::UnorderedElementsAre;
+
 class HeadersTest : public ::testing::Test {
 public:
   HeadersTest() {
 CDB.ExtraClangFlags = {SearchDirArg.c_str()};
 FS.Files[MainFile] = "";
+// Make sure directory sub/ exists.
+FS.Files[testPath("sub/EMPTY")] = "";
+  }
+
+private:
+  std::unique_ptr setupClang() {
+auto Cmd = CDB.getCompileCommand(MainFile);
+assert(static_cast(Cmd));
+auto VFS = FS.getFileSystem();
+VFS->setCurrentWorkingDirectory(Cmd->Directory);
+
+std::vector Argv;
+for (const auto &S : Cmd->CommandLine)
+  Argv.push_back(S.c_str());
+auto CI = clang::createInvocationFromCommandLine(
+Argv,
+CompilerInstance::createDiagnostics(new DiagnosticOptions(),
+&IgnoreDiags, false),
+VFS);
+EXPECT_TRUE(static_cast(CI));
+CI->getFrontendOpts().DisableFree = false;
+
+// The diagnostic options must be set before creating a CompilerInstance.
+CI->getDiagnosticOpts().IgnoreWarnings = true;
+auto Clang = prepareCompilerInstance(
+std::move(CI), /*Preamble=*/nullptr,
+llvm::MemoryBuffer::getMemBuffer(FS.Files[MainFile], MainFile),
+std::make_shared(), VFS, IgnoreDiags);
+
+EXPECT_FALSE(Clang->getFrontendOpts().Inputs.empty());
+return Clang;
   }
 
 protected:
+  std::vector collectIncludes() {
+auto Clang = setupClang();
+PreprocessOnlyAction Action;
+EXPECT_TRUE(
+Action.BeginSourceFile(*Clang, Clang->getFrontendOpts().Inputs[0]));
+std::vector Inclusions;
+Clang->getPreprocessor().addPPCallbacks(collectInclusionsInMainFileCallback(
+Clang->getSourceManager(), Inclusions));
+EXPECT_TRUE(Action.Execute());
+Action.EndSourceFile();
+return Inclusions;
+  }
+
   // Calculates the include path, or returns "" on error.
   std::string calculate(PathRef Original, PathRef Preferred = "",
+const std::vector &Inclusions = {},
 bool ExpectError = false) {
+auto Clang = setupClang();
+PreprocessOnlyAction Action;
+EXPECT_TRUE(
+Action.BeginSourceFile(*Clang, Clang->getFrontendOpts().Inputs[0]));
+
 if (Preferred.empty())
   Preferred = Original;
-auto VFS = FS.getFileSystem();
-auto Cmd = CDB.getCompileCommand(MainFile);
-assert(static_cast(Cmd));
-VFS->setCurrentWorkingDirectory(Cmd->Directory);
 auto ToHeaderFile = [](llvm::StringRef Header) {
   return HeaderFile{Header,
 /*Verbatim=*/!llvm::sys::path::is_absolute(Header)};
 };
-auto Path = calculateIncludePath(MainFile, FS.Files[MainFile],
- ToHeaderFile(Original),
- ToHeaderFile(Preferred), *Cmd, VFS);
+
+auto Path = calculateIncludePath(
+MainFile, CDB.getCompileCommand(MainFile)->Directory,
+Clang->getPreprocessor().getHeaderSearchInfo(), Inclusions,
+ToHeaderFile(Original), ToHeaderFile(Preferred));
+Action.EndSourceFile();
 if (!Path) {
   llvm::consumeError(Path.takeError());
   EXPECT_TRUE(ExpectError);
@@ -49,52 +107,31 @@
 }
 return std::move(*Path);
   }
+
   MockFSProvider FS;
   MockCompilationDatabase CDB;
   std::string MainFile = testPath("main.cpp");
   std::string Subdir = testPath("sub");
   std::string SearchDirArg = (llvm::Twine("-I") + Subdir).str();
+  IgnoringDiagConsumer IgnoreDiags;
 };
 
-TEST_F(HeadersTest, InsertInclude) {
-  std::string Path = testPath("sub/bar.h");
-  FS.Files[Path] = "";
-  EXPECT_EQ(c

[PATCH] D46670: [clangd] Move helpers that convert Replacements to TextEdits to SourceCode.h

[Eric Liu via Phabricator via cfe-commits]

ioeric created this revision.
ioeric added a reviewer: ilya-biryukov.
Herald added subscribers: cfe-commits, jkorous, MaskRay, klimek.

Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D46670

Files:
  clangd/ClangdLSPServer.cpp
  clangd/SourceCode.cpp
  clangd/SourceCode.h


Index: clangd/SourceCode.h
===
--- clangd/SourceCode.h
+++ clangd/SourceCode.h
@@ -15,6 +15,7 @@
 #define LLVM_CLANG_TOOLS_EXTRA_CLANGD_SOURCECODE_H
 #include "Protocol.h"
 #include "clang/Basic/SourceLocation.h"
+#include "clang/Tooling/Core/Replacement.h"
 
 namespace clang {
 class SourceManager;
@@ -54,6 +55,14 @@
 /// qualifier.
 std::pair
 splitQualifiedName(llvm::StringRef QName);
+TextEdit replacementToEdit(StringRef Code, const tooling::Replacement &R);
+
+std::vector
+replacementsToEdits(StringRef Code,
+const std::vector &Replacements);
+
+std::vector replacementsToEdits(StringRef Code,
+  const tooling::Replacements &Repls);
 
 } // namespace clangd
 } // namespace clang
Index: clangd/SourceCode.cpp
===
--- clangd/SourceCode.cpp
+++ clangd/SourceCode.cpp
@@ -166,5 +166,31 @@
   return {QName.substr(0, Pos + 2), QName.substr(Pos + 2)};
 }
 
+TextEdit replacementToEdit(StringRef Code, const tooling::Replacement &R) {
+  Range ReplacementRange = {
+  offsetToPosition(Code, R.getOffset()),
+  offsetToPosition(Code, R.getOffset() + R.getLength())};
+  return {ReplacementRange, R.getReplacementText()};
+}
+
+std::vector
+replacementsToEdits(StringRef Code,
+const std::vector &Replacements) {
+  // Turn the replacements into the format specified by the Language Server
+  // Protocol. Fuse them into one big JSON array.
+  std::vector Edits;
+  for (const auto &R : Replacements)
+Edits.push_back(replacementToEdit(Code, R));
+  return Edits;
+}
+
+std::vector replacementsToEdits(StringRef Code,
+  const tooling::Replacements &Repls) {
+  std::vector Edits;
+  for (const auto &R : Repls)
+Edits.push_back(replacementToEdit(Code, R));
+  return Edits;
+}
+
 } // namespace clangd
 } // namespace clang
Index: clangd/ClangdLSPServer.cpp
===
--- clangd/ClangdLSPServer.cpp
+++ clangd/ClangdLSPServer.cpp
@@ -60,32 +60,6 @@
 static URISchemeRegistry::Add
 X("test", "Test scheme for clangd lit tests.");
 
-TextEdit replacementToEdit(StringRef Code, const tooling::Replacement &R) {
-  Range ReplacementRange = {
-  offsetToPosition(Code, R.getOffset()),
-  offsetToPosition(Code, R.getOffset() + R.getLength())};
-  return {ReplacementRange, R.getReplacementText()};
-}
-
-std::vector
-replacementsToEdits(StringRef Code,
-const std::vector &Replacements) {
-  // Turn the replacements into the format specified by the Language Server
-  // Protocol. Fuse them into one big JSON array.
-  std::vector Edits;
-  for (const auto &R : Replacements)
-Edits.push_back(replacementToEdit(Code, R));
-  return Edits;
-}
-
-std::vector replacementsToEdits(StringRef Code,
-  const tooling::Replacements &Repls) {
-  std::vector Edits;
-  for (const auto &R : Repls)
-Edits.push_back(replacementToEdit(Code, R));
-  return Edits;
-}
-
 SymbolKindBitset defaultSymbolKinds() {
   SymbolKindBitset Defaults;
   for (size_t I = SymbolKindMin; I <= static_cast(SymbolKind::Array);


Index: clangd/SourceCode.h
===
--- clangd/SourceCode.h
+++ clangd/SourceCode.h
@@ -15,6 +15,7 @@
 #define LLVM_CLANG_TOOLS_EXTRA_CLANGD_SOURCECODE_H
 #include "Protocol.h"
 #include "clang/Basic/SourceLocation.h"
+#include "clang/Tooling/Core/Replacement.h"
 
 namespace clang {
 class SourceManager;
@@ -54,6 +55,14 @@
 /// qualifier.
 std::pair
 splitQualifiedName(llvm::StringRef QName);
+TextEdit replacementToEdit(StringRef Code, const tooling::Replacement &R);
+
+std::vector
+replacementsToEdits(StringRef Code,
+const std::vector &Replacements);
+
+std::vector replacementsToEdits(StringRef Code,
+  const tooling::Replacements &Repls);
 
 } // namespace clangd
 } // namespace clang
Index: clangd/SourceCode.cpp
===
--- clangd/SourceCode.cpp
+++ clangd/SourceCode.cpp
@@ -166,5 +166,31 @@
   return {QName.substr(0, Pos + 2), QName.substr(Pos + 2)};
 }
 
+TextEdit replacementToEdit(StringRef Code, const tooling::Replacement &R) {
+  Range ReplacementRange = {
+  offsetToPosition(Code, R.getOffset()),
+  offsetToPosition(Code, R.getOffset() + R.getLength())};
+  return {ReplacementRange, R.getReplacementText()};
+}
+
+std::vector
+replacementsToEdits(StringRef Code,
+const st

[PATCH] D46675: [clangd] Add helper for collecting #include directives in file.

[Eric Liu via Phabricator via cfe-commits]

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

Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D46675

Files:
  clangd/ClangdUnit.cpp
  clangd/ClangdUnit.h
  clangd/Headers.cpp
  clangd/Headers.h
  clangd/XRefs.cpp

Index: clangd/XRefs.cpp
===
--- clangd/XRefs.cpp
+++ clangd/XRefs.cpp
@@ -234,12 +234,10 @@
 
   std::vector Result;
   // Handle goto definition for #include.
-  for (auto &IncludeLoc : AST.getInclusionLocations()) {
-Range R = IncludeLoc.first;
+  for (auto &Inc : AST.getInclusions()) {
 Position Pos = sourceLocToPosition(SourceMgr, SourceLocationBeg);
-
-if (R.contains(Pos))
-  Result.push_back(Location{URIForFile{IncludeLoc.second}, {}});
+if (!Inc.Resolved.empty() && Inc.R.contains(Pos))
+  Result.push_back(Location{URIForFile{Inc.Resolved}, {}});
   }
   if (!Result.empty())
 return Result;
Index: clangd/Headers.h
===
--- clangd/Headers.h
+++ clangd/Headers.h
@@ -11,7 +11,9 @@
 #define LLVM_CLANG_TOOLS_EXTRA_CLANGD_HEADERS_H
 
 #include "Path.h"
+#include "Protocol.h"
 #include "clang/Basic/VirtualFileSystem.h"
+#include "clang/Lex/PPCallbacks.h"
 #include "clang/Tooling/CompilationDatabase.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/Support/Error.h"
@@ -32,6 +34,19 @@
   bool valid() const;
 };
 
+// An #include directive that we found in the main file.
+struct Inclusion {
+  Range R; // Inclusion range.
+  std::string Written; // Inclusion name as written e.g. .
+  Path Resolved;   // Resolved path of included file. Empty if not resolved.
+};
+
+/// Returns a PPCallback that collects all inclusions in the main file.
+/// Inclusions are added to \p Includes.
+std::unique_ptr
+collectInclusionsInMainFileCallback(const SourceManager &SM,
+std::vector &Inclusions);
+
 /// 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'.
Index: clangd/Headers.cpp
===
--- clangd/Headers.cpp
+++ clangd/Headers.cpp
@@ -10,6 +10,7 @@
 #include "Headers.h"
 #include "Compiler.h"
 #include "Logger.h"
+#include "SourceCode.h"
 #include "clang/Frontend/CompilerInstance.h"
 #include "clang/Frontend/CompilerInvocation.h"
 #include "clang/Frontend/FrontendActions.h"
@@ -24,26 +25,32 @@
 
 class RecordHeaders : public PPCallbacks {
 public:
-  RecordHeaders(llvm::StringSet<> &WrittenHeaders,
-llvm::StringSet<> &ResolvedHeaders)
-  : WrittenHeaders(WrittenHeaders), ResolvedHeaders(ResolvedHeaders) {}
+  RecordHeaders(const SourceManager &SM, std::vector &Includes)
+  : SM(SM), Includes(Includes) {}
 
-  void InclusionDirective(SourceLocation /*HashLoc*/,
-  const Token & /*IncludeTok*/,
+  // Record existing #includes - both written and resolved paths. Only #includes
+  // in the main file are collected.
+  void InclusionDirective(SourceLocation HashLoc, const Token & /*IncludeTok*/,
   llvm::StringRef FileName, bool IsAngled,
-  CharSourceRange /*FilenameRange*/,
-  const FileEntry *File, llvm::StringRef /*SearchPath*/,
+  CharSourceRange FilenameRange, const FileEntry *File,
+  llvm::StringRef /*SearchPath*/,
   llvm::StringRef /*RelativePath*/,
   const Module * /*Imported*/) override {
-WrittenHeaders.insert(
-(IsAngled ? "<" + FileName + ">" : "\"" + FileName + "\"").str());
-if (File != nullptr && !File->tryGetRealPathName().empty())
-  ResolvedHeaders.insert(File->tryGetRealPathName());
+// Only inclusion directives in the main file make sense. The user cannot
+// select directives not in the main file.
+if (HashLoc.isInvalid() || !SM.isInMainFile(HashLoc))
+  return;
+std::string Written =
+(IsAngled ? "<" + FileName + ">" : "\"" + FileName + "\"").str();
+std::string Resolved = (!File || File->tryGetRealPathName().empty())
+   ? ""
+   : File->tryGetRealPathName();
+Includes.push_back({halfOpenToRange(SM, FilenameRange), Written, Resolved});
   }
 
 private:
-  llvm::StringSet<> &WrittenHeaders;
-  llvm::StringSet<> &ResolvedHeaders;
+  const SourceManager &SM;
+  std::vector &Includes;
 };
 
 } // namespace
@@ -57,6 +64,12 @@
  (!Verbatim && llvm::sys::path::is_absolute(File));
 }
 
+std::unique_ptr
+collectInclusionsInMainFileCallback(const SourceManager &SM,
+std::vector &Includes) {
+  return llvm

[PATCH] D46675: [clangd] Add helper for collecting #include directives in file.

[Eric Liu via Phabricator via cfe-commits]

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

- Rebase on origin/master


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D46675

Files:
  clangd/ClangdLSPServer.cpp
  clangd/ClangdUnit.cpp
  clangd/ClangdUnit.h
  clangd/Headers.cpp
  clangd/Headers.h
  clangd/SourceCode.cpp
  clangd/SourceCode.h
  clangd/XRefs.cpp

Index: clangd/XRefs.cpp
===
--- clangd/XRefs.cpp
+++ clangd/XRefs.cpp
@@ -234,12 +234,10 @@
 
   std::vector Result;
   // Handle goto definition for #include.
-  for (auto &IncludeLoc : AST.getInclusionLocations()) {
-Range R = IncludeLoc.first;
+  for (auto &Inc : AST.getInclusions()) {
 Position Pos = sourceLocToPosition(SourceMgr, SourceLocationBeg);
-
-if (R.contains(Pos))
-  Result.push_back(Location{URIForFile{IncludeLoc.second}, {}});
+if (!Inc.Resolved.empty() && Inc.R.contains(Pos))
+  Result.push_back(Location{URIForFile{Inc.Resolved}, {}});
   }
   if (!Result.empty())
 return Result;
Index: clangd/SourceCode.h
===
--- clangd/SourceCode.h
+++ clangd/SourceCode.h
@@ -15,6 +15,7 @@
 #define LLVM_CLANG_TOOLS_EXTRA_CLANGD_SOURCECODE_H
 #include "Protocol.h"
 #include "clang/Basic/SourceLocation.h"
+#include "clang/Tooling/Core/Replacement.h"
 
 namespace clang {
 class SourceManager;
@@ -54,6 +55,14 @@
 /// qualifier.
 std::pair
 splitQualifiedName(llvm::StringRef QName);
+TextEdit replacementToEdit(StringRef Code, const tooling::Replacement &R);
+
+std::vector
+replacementsToEdits(StringRef Code,
+const std::vector &Replacements);
+
+std::vector replacementsToEdits(StringRef Code,
+  const tooling::Replacements &Repls);
 
 } // namespace clangd
 } // namespace clang
Index: clangd/SourceCode.cpp
===
--- clangd/SourceCode.cpp
+++ clangd/SourceCode.cpp
@@ -166,5 +166,31 @@
   return {QName.substr(0, Pos + 2), QName.substr(Pos + 2)};
 }
 
+TextEdit replacementToEdit(StringRef Code, const tooling::Replacement &R) {
+  Range ReplacementRange = {
+  offsetToPosition(Code, R.getOffset()),
+  offsetToPosition(Code, R.getOffset() + R.getLength())};
+  return {ReplacementRange, R.getReplacementText()};
+}
+
+std::vector
+replacementsToEdits(StringRef Code,
+const std::vector &Replacements) {
+  // Turn the replacements into the format specified by the Language Server
+  // Protocol. Fuse them into one big JSON array.
+  std::vector Edits;
+  for (const auto &R : Replacements)
+Edits.push_back(replacementToEdit(Code, R));
+  return Edits;
+}
+
+std::vector replacementsToEdits(StringRef Code,
+  const tooling::Replacements &Repls) {
+  std::vector Edits;
+  for (const auto &R : Repls)
+Edits.push_back(replacementToEdit(Code, R));
+  return Edits;
+}
+
 } // namespace clangd
 } // namespace clang
Index: clangd/Headers.h
===
--- clangd/Headers.h
+++ clangd/Headers.h
@@ -11,7 +11,9 @@
 #define LLVM_CLANG_TOOLS_EXTRA_CLANGD_HEADERS_H
 
 #include "Path.h"
+#include "Protocol.h"
 #include "clang/Basic/VirtualFileSystem.h"
+#include "clang/Lex/PPCallbacks.h"
 #include "clang/Tooling/CompilationDatabase.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/Support/Error.h"
@@ -32,6 +34,19 @@
   bool valid() const;
 };
 
+// An #include directive that we found in the main file.
+struct Inclusion {
+  Range R; // Inclusion range.
+  std::string Written; // Inclusion name as written e.g. .
+  Path Resolved;   // Resolved path of included file. Empty if not resolved.
+};
+
+/// Returns a PPCallback that collects all inclusions in the main file.
+/// Inclusions are added to \p Includes.
+std::unique_ptr
+collectInclusionsInMainFileCallback(const SourceManager &SM,
+std::vector &Inclusions);
+
 /// 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'.
Index: clangd/Headers.cpp
===
--- clangd/Headers.cpp
+++ clangd/Headers.cpp
@@ -10,6 +10,7 @@
 #include "Headers.h"
 #include "Compiler.h"
 #include "Logger.h"
+#include "SourceCode.h"
 #include "clang/Frontend/CompilerInstance.h"
 #include "clang/Frontend/CompilerInvocation.h"
 #include "clang/Frontend/FrontendActions.h"
@@ -24,26 +25,32 @@
 
 class RecordHeaders : public PPCallbacks {
 public:
-  RecordHeaders(llvm::StringSet<> &WrittenHeaders,
-llvm::StringSet<> &ResolvedHeaders)
-  : WrittenHeaders(WrittenHeaders), ResolvedHeaders(ResolvedHeaders) {}
+  RecordHeaders(const SourceManager &SM, std::vector &Includes)
+  : SM

[PATCH] D46675: [clangd] Add helper for collecting #include directives in file.

[Eric Liu via Phabricator via cfe-commits]

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

- Unmerge https://reviews.llvm.org/D46670


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D46675

Files:
  clangd/ClangdUnit.cpp
  clangd/ClangdUnit.h
  clangd/Headers.cpp
  clangd/Headers.h
  clangd/XRefs.cpp

Index: clangd/XRefs.cpp
===
--- clangd/XRefs.cpp
+++ clangd/XRefs.cpp
@@ -234,12 +234,10 @@
 
   std::vector Result;
   // Handle goto definition for #include.
-  for (auto &IncludeLoc : AST.getInclusionLocations()) {
-Range R = IncludeLoc.first;
+  for (auto &Inc : AST.getInclusions()) {
 Position Pos = sourceLocToPosition(SourceMgr, SourceLocationBeg);
-
-if (R.contains(Pos))
-  Result.push_back(Location{URIForFile{IncludeLoc.second}, {}});
+if (!Inc.Resolved.empty() && Inc.R.contains(Pos))
+  Result.push_back(Location{URIForFile{Inc.Resolved}, {}});
   }
   if (!Result.empty())
 return Result;
Index: clangd/Headers.h
===
--- clangd/Headers.h
+++ clangd/Headers.h
@@ -11,7 +11,9 @@
 #define LLVM_CLANG_TOOLS_EXTRA_CLANGD_HEADERS_H
 
 #include "Path.h"
+#include "Protocol.h"
 #include "clang/Basic/VirtualFileSystem.h"
+#include "clang/Lex/PPCallbacks.h"
 #include "clang/Tooling/CompilationDatabase.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/Support/Error.h"
@@ -32,6 +34,19 @@
   bool valid() const;
 };
 
+// An #include directive that we found in the main file.
+struct Inclusion {
+  Range R; // Inclusion range.
+  std::string Written; // Inclusion name as written e.g. .
+  Path Resolved;   // Resolved path of included file. Empty if not resolved.
+};
+
+/// Returns a PPCallback that collects all inclusions in the main file.
+/// Inclusions are added to \p Includes.
+std::unique_ptr
+collectInclusionsInMainFileCallback(const SourceManager &SM,
+std::vector &Inclusions);
+
 /// 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'.
Index: clangd/Headers.cpp
===
--- clangd/Headers.cpp
+++ clangd/Headers.cpp
@@ -10,6 +10,7 @@
 #include "Headers.h"
 #include "Compiler.h"
 #include "Logger.h"
+#include "SourceCode.h"
 #include "clang/Frontend/CompilerInstance.h"
 #include "clang/Frontend/CompilerInvocation.h"
 #include "clang/Frontend/FrontendActions.h"
@@ -24,26 +25,32 @@
 
 class RecordHeaders : public PPCallbacks {
 public:
-  RecordHeaders(llvm::StringSet<> &WrittenHeaders,
-llvm::StringSet<> &ResolvedHeaders)
-  : WrittenHeaders(WrittenHeaders), ResolvedHeaders(ResolvedHeaders) {}
+  RecordHeaders(const SourceManager &SM, std::vector &Includes)
+  : SM(SM), Includes(Includes) {}
 
-  void InclusionDirective(SourceLocation /*HashLoc*/,
-  const Token & /*IncludeTok*/,
+  // Record existing #includes - both written and resolved paths. Only #includes
+  // in the main file are collected.
+  void InclusionDirective(SourceLocation HashLoc, const Token & /*IncludeTok*/,
   llvm::StringRef FileName, bool IsAngled,
-  CharSourceRange /*FilenameRange*/,
-  const FileEntry *File, llvm::StringRef /*SearchPath*/,
+  CharSourceRange FilenameRange, const FileEntry *File,
+  llvm::StringRef /*SearchPath*/,
   llvm::StringRef /*RelativePath*/,
   const Module * /*Imported*/) override {
-WrittenHeaders.insert(
-(IsAngled ? "<" + FileName + ">" : "\"" + FileName + "\"").str());
-if (File != nullptr && !File->tryGetRealPathName().empty())
-  ResolvedHeaders.insert(File->tryGetRealPathName());
+// Only inclusion directives in the main file make sense. The user cannot
+// select directives not in the main file.
+if (HashLoc.isInvalid() || !SM.isInMainFile(HashLoc))
+  return;
+std::string Written =
+(IsAngled ? "<" + FileName + ">" : "\"" + FileName + "\"").str();
+std::string Resolved = (!File || File->tryGetRealPathName().empty())
+   ? ""
+   : File->tryGetRealPathName();
+Includes.push_back({halfOpenToRange(SM, FilenameRange), Written, Resolved});
   }
 
 private:
-  llvm::StringSet<> &WrittenHeaders;
-  llvm::StringSet<> &ResolvedHeaders;
+  const SourceManager &SM;
+  std::vector &Includes;
 };
 
 } // namespace
@@ -57,6 +64,12 @@
  (!Verbatim && llvm::sys::path::is_absolute(File));
 }
 
+std::unique_ptr
+collectInclusionsInMainFileCallback(const SourceManager &SM,
+std::vector &Includes) {
+  return llvm::make_unique(SM, Includes);
+}
+

[PATCH] D46676: [clangd] Remove LSP command-based #include insertion.

[Eric Liu via Phabricator via cfe-commits]

ioeric created this revision.
ioeric added reviewers: ilya-biryukov, sammccall.
Herald added subscribers: cfe-commits, jkorous, MaskRay, klimek.

clangd will populate #include insertions as addtionalEdits in completion
items.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D46676

Files:
  clangd/ClangdLSPServer.cpp
  clangd/ClangdServer.cpp
  clangd/ClangdServer.h
  clangd/CodeComplete.cpp
  clangd/Protocol.cpp
  clangd/Protocol.h
  test/clangd/initialize-params-invalid.test
  test/clangd/initialize-params.test
  test/clangd/insert-include.test
  unittests/clangd/ClangdTests.cpp

Index: unittests/clangd/ClangdTests.cpp
===
--- unittests/clangd/ClangdTests.cpp
+++ unittests/clangd/ClangdTests.cpp
@@ -938,67 +938,6 @@
   ASSERT_EQ(DiagConsumer.Count, 2); // Sanity check - we actually ran both?
 }
 
-TEST_F(ClangdVFSTest, InsertIncludes) {
-  MockFSProvider FS;
-  ErrorCheckingDiagConsumer DiagConsumer;
-  MockCompilationDatabase CDB;
-  std::string SearchDirArg = (llvm::Twine("-I") + testRoot()).str();
-  CDB.ExtraClangFlags.insert(CDB.ExtraClangFlags.end(), {SearchDirArg.c_str()});
-  ClangdServer Server(CDB, FS, DiagConsumer, ClangdServer::optsForTest());
-
-  auto FooCpp = testPath("foo.cpp");
-  const auto Code = R"cpp(
-#include "x.h"
-#include "z.h"
-
-void f() {}
-)cpp";
-  FS.Files[FooCpp] = Code;
-  runAddDocument(Server, FooCpp, Code);
-
-  auto ChangedCode = [&](llvm::StringRef Original, llvm::StringRef Preferred) {
-auto Replaces = Server.insertInclude(
-FooCpp, Code, Original, Preferred.empty() ? Original : Preferred);
-EXPECT_TRUE(static_cast(Replaces));
-auto Changed = tooling::applyAllReplacements(Code, *Replaces);
-EXPECT_TRUE(static_cast(Changed));
-return *Changed;
-  };
-  auto Inserted = [&](llvm::StringRef Original, llvm::StringRef Preferred,
-  llvm::StringRef Expected) {
-return llvm::StringRef(ChangedCode(Original, Preferred))
-.contains((llvm::Twine("#include ") + Expected + "").str());
-  };
-
-  EXPECT_TRUE(Inserted("\"y.h\"", /*Preferred=*/"", "\"y.h\""));
-  EXPECT_TRUE(Inserted("\"y.h\"", /*Preferred=*/"\"Y.h\"", "\"Y.h\""));
-  EXPECT_TRUE(Inserted("", /*Preferred=*/"", ""));
-  EXPECT_TRUE(Inserted("", /*Preferred=*/"", ""));
-
-  std::string OriginalHeader = URI::createFile(testPath("y.h")).toString();
-  std::string PreferredHeader = URI::createFile(testPath("Y.h")).toString();
-  EXPECT_TRUE(Inserted(OriginalHeader,
-   /*Preferred=*/"", "\"y.h\""));
-  EXPECT_TRUE(Inserted(OriginalHeader,
-   /*Preferred=*/"", ""));
-  EXPECT_TRUE(Inserted(OriginalHeader, PreferredHeader, "\"Y.h\""));
-  EXPECT_TRUE(Inserted("", PreferredHeader, "\"Y.h\""));
-  auto TestURIHeader =
-  URI::parse(llvm::formatv("{0}:///x/y/z.h", ClangdTestScheme).str());
-  EXPECT_TRUE(static_cast(TestURIHeader));
-  EXPECT_TRUE(Inserted(TestURIHeader->toString(), "", "\"x/y/z.h\""));
-
-  // Check that includes are sorted.
-  const auto Expected = R"cpp(
-#include "x.h"
-#include "y.h"
-#include "z.h"
-
-void f() {}
-)cpp";
-  EXPECT_EQ(Expected, ChangedCode("\"y.h\"", /*Preferred=*/""));
-}
-
 TEST_F(ClangdVFSTest, FormatCode) {
   MockFSProvider FS;
   ErrorCheckingDiagConsumer DiagConsumer;
Index: test/clangd/insert-include.test
===
--- test/clangd/insert-include.test
+++ /dev/null
@@ -1,36 +0,0 @@
-# RUN: clangd -lit-test < %s | FileCheck -strict-whitespace %s
-# RUN: clangd -lit-test -pch-storage=memory < %s | FileCheck -strict-whitespace %s
-{"jsonrpc":"2.0","id":0,"method":"initialize","params":{"processId":123,"rootPath":"clangd","capabilities":{},"trace":"off"}}

-{"jsonrpc":"2.0","method":"textDocument/didOpen","params":{"textDocument":{"uri":"test:///main.cpp","languageId":"cpp","version":1,"text":"void f() {}"}}}

-{"jsonrpc":"2.0","id":3,"method":"workspace/executeCommand","params":{"arguments":[{"declaringHeader":"\"/usr/include/bits/vector\"", "preferredHeader":"","textDocument":{"uri":"test:///main.cpp"}}],"command":"clangd.insertInclude"}}
-#  CHECK:"id": 3,
-# CHECK-NEXT:"jsonrpc": "2.0",
-# CHECK-NEXT:"result": "Inserted header \"/usr/include/bits/vector\" ()"
-# CHECK-NEXT:  }
-#  CHECK:"method": "workspace/applyEdit",
-# CHECK-NEXT:"params": {
-# CHECK-NEXT:  "edit": {
-# CHECK-NEXT:"changes": {
-# CHECK-NEXT:  "file://{{.*}}/main.cpp": [
-# CHECK-NEXT:{
-# CHECK-NEXT:  "newText": "#include \n",
-# CHECK-NEXT:  "range": {
-# CHECK-NEXT:"end": {
-# CHECK-NEXT:  "character": 0,
-# CHECK-NEXT:  "line": 0
-# CHECK-NEXT:},
-# CHECK-NEXT:"start": {
-# CHECK-NEXT:  "character": 0,
-# CHECK-NEXT:  "line": 0
-# CHECK-NEXT:}
-# CHECK-

[PATCH] D46670: [clangd] Move helpers that convert Replacements to TextEdits to SourceCode.h

[Eric Liu via Phabricator via cfe-commits]

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

- [clangd] Add helper for collecting #include directives in file.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D46670

Files:
  clangd/ClangdLSPServer.cpp
  clangd/SourceCode.cpp
  clangd/SourceCode.h

Index: clangd/SourceCode.h
===
--- clangd/SourceCode.h
+++ clangd/SourceCode.h
@@ -15,6 +15,7 @@
 #define LLVM_CLANG_TOOLS_EXTRA_CLANGD_SOURCECODE_H
 #include "Protocol.h"
 #include "clang/Basic/SourceLocation.h"
+#include "clang/Tooling/Core/Replacement.h"
 
 namespace clang {
 class SourceManager;
@@ -55,6 +56,15 @@
 std::pair
 splitQualifiedName(llvm::StringRef QName);
 
+TextEdit replacementToEdit(StringRef Code, const tooling::Replacement &R);
+
+std::vector
+replacementsToEdits(StringRef Code,
+const std::vector &Replacements);
+
+std::vector replacementsToEdits(StringRef Code,
+  const tooling::Replacements &Repls);
+
 } // namespace clangd
 } // namespace clang
 #endif
Index: clangd/SourceCode.cpp
===
--- clangd/SourceCode.cpp
+++ clangd/SourceCode.cpp
@@ -166,5 +166,31 @@
   return {QName.substr(0, Pos + 2), QName.substr(Pos + 2)};
 }
 
+TextEdit replacementToEdit(StringRef Code, const tooling::Replacement &R) {
+  Range ReplacementRange = {
+  offsetToPosition(Code, R.getOffset()),
+  offsetToPosition(Code, R.getOffset() + R.getLength())};
+  return {ReplacementRange, R.getReplacementText()};
+}
+
+std::vector
+replacementsToEdits(StringRef Code,
+const std::vector &Replacements) {
+  // Turn the replacements into the format specified by the Language Server
+  // Protocol. Fuse them into one big JSON array.
+  std::vector Edits;
+  for (const auto &R : Replacements)
+Edits.push_back(replacementToEdit(Code, R));
+  return Edits;
+}
+
+std::vector replacementsToEdits(StringRef Code,
+  const tooling::Replacements &Repls) {
+  std::vector Edits;
+  for (const auto &R : Repls)
+Edits.push_back(replacementToEdit(Code, R));
+  return Edits;
+}
+
 } // namespace clangd
 } // namespace clang
Index: clangd/ClangdLSPServer.cpp
===
--- clangd/ClangdLSPServer.cpp
+++ clangd/ClangdLSPServer.cpp
@@ -60,32 +60,6 @@
 static URISchemeRegistry::Add
 X("test", "Test scheme for clangd lit tests.");
 
-TextEdit replacementToEdit(StringRef Code, const tooling::Replacement &R) {
-  Range ReplacementRange = {
-  offsetToPosition(Code, R.getOffset()),
-  offsetToPosition(Code, R.getOffset() + R.getLength())};
-  return {ReplacementRange, R.getReplacementText()};
-}
-
-std::vector
-replacementsToEdits(StringRef Code,
-const std::vector &Replacements) {
-  // Turn the replacements into the format specified by the Language Server
-  // Protocol. Fuse them into one big JSON array.
-  std::vector Edits;
-  for (const auto &R : Replacements)
-Edits.push_back(replacementToEdit(Code, R));
-  return Edits;
-}
-
-std::vector replacementsToEdits(StringRef Code,
-  const tooling::Replacements &Repls) {
-  std::vector Edits;
-  for (const auto &R : Repls)
-Edits.push_back(replacementToEdit(Code, R));
-  return Edits;
-}
-
 SymbolKindBitset defaultSymbolKinds() {
   SymbolKindBitset Defaults;
   for (size_t I = SymbolKindMin; I <= static_cast(SymbolKind::Array);
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D46497: [clangd] Populate #include insertions as additional edits in completion items.

[Eric Liu via Phabricator via cfe-commits]

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

- Rebase on https://reviews.llvm.org/D46670, https://reviews.llvm.org/D46675, 
https://reviews.llvm.org/D46676


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D46497

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

Index: unittests/clangd/HeadersTests.cpp
===
--- unittests/clangd/HeadersTests.cpp
+++ unittests/clangd/HeadersTests.cpp
@@ -8,38 +8,96 @@
 //===--===//
 
 #include "Headers.h"
+
+#include "Compiler.h"
 #include "TestFS.h"
+#include "clang/Frontend/CompilerInstance.h"
+#include "clang/Frontend/CompilerInvocation.h"
+#include "clang/Frontend/FrontendActions.h"
+#include "clang/Lex/PreprocessorOptions.h"
 #include "gmock/gmock.h"
 #include "gtest/gtest.h"
 
 namespace clang {
 namespace clangd {
 namespace {
 
+using ::testing::AllOf;
+using ::testing::UnorderedElementsAre;
+
 class HeadersTest : public ::testing::Test {
 public:
   HeadersTest() {
 CDB.ExtraClangFlags = {SearchDirArg.c_str()};
 FS.Files[MainFile] = "";
+// Make sure directory sub/ exists.
+FS.Files[testPath("sub/EMPTY")] = "";
+  }
+
+private:
+  std::unique_ptr setupClang() {
+auto Cmd = CDB.getCompileCommand(MainFile);
+assert(static_cast(Cmd));
+auto VFS = FS.getFileSystem();
+VFS->setCurrentWorkingDirectory(Cmd->Directory);
+
+std::vector Argv;
+for (const auto &S : Cmd->CommandLine)
+  Argv.push_back(S.c_str());
+auto CI = clang::createInvocationFromCommandLine(
+Argv,
+CompilerInstance::createDiagnostics(new DiagnosticOptions(),
+&IgnoreDiags, false),
+VFS);
+EXPECT_TRUE(static_cast(CI));
+CI->getFrontendOpts().DisableFree = false;
+
+// The diagnostic options must be set before creating a CompilerInstance.
+CI->getDiagnosticOpts().IgnoreWarnings = true;
+auto Clang = prepareCompilerInstance(
+std::move(CI), /*Preamble=*/nullptr,
+llvm::MemoryBuffer::getMemBuffer(FS.Files[MainFile], MainFile),
+std::make_shared(), VFS, IgnoreDiags);
+
+EXPECT_FALSE(Clang->getFrontendOpts().Inputs.empty());
+return Clang;
   }
 
 protected:
+  std::vector collectIncludes() {
+auto Clang = setupClang();
+PreprocessOnlyAction Action;
+EXPECT_TRUE(
+Action.BeginSourceFile(*Clang, Clang->getFrontendOpts().Inputs[0]));
+std::vector Inclusions;
+Clang->getPreprocessor().addPPCallbacks(collectInclusionsInMainFileCallback(
+Clang->getSourceManager(), Inclusions));
+EXPECT_TRUE(Action.Execute());
+Action.EndSourceFile();
+return Inclusions;
+  }
+
   // Calculates the include path, or returns "" on error.
   std::string calculate(PathRef Original, PathRef Preferred = "",
+const std::vector &Inclusions = {},
 bool ExpectError = false) {
+auto Clang = setupClang();
+PreprocessOnlyAction Action;
+EXPECT_TRUE(
+Action.BeginSourceFile(*Clang, Clang->getFrontendOpts().Inputs[0]));
+
 if (Preferred.empty())
   Preferred = Original;
-auto VFS = FS.getFileSystem();
-auto Cmd = CDB.getCompileCommand(MainFile);
-assert(static_cast(Cmd));
-VFS->setCurrentWorkingDirectory(Cmd->Directory);
 auto ToHeaderFile = [](llvm::StringRef Header) {
   return HeaderFile{Header,
 /*Verbatim=*/!llvm::sys::path::is_absolute(Header)};
 };
-auto Path = calculateIncludePath(MainFile, FS.Files[MainFile],
- ToHeaderFile(Original),
- ToHeaderFile(Preferred), *Cmd, VFS);
+
+auto Path = calculateIncludePath(
+MainFile, CDB.getCompileCommand(MainFile)->Directory,
+Clang->getPreprocessor().getHeaderSearchInfo(), Inclusions,
+ToHeaderFile(Original), ToHeaderFile(Preferred));
+Action.EndSourceFile();
 if (!Path) {
   llvm::consumeError(Path.takeError());
   EXPECT_TRUE(ExpectError);
@@ -49,52 +107,31 @@
 }
 return std::move(*Path);
   }
+
   MockFSProvider FS;
   MockCompilationDatabase CDB;
   std::string MainFile = testPath("main.cpp");
   std::string Subdir = testPath("sub");
   std::string SearchDirArg = (llvm::Twine("-I") + Subdir).str();
+  IgnoringDiagConsumer IgnoreDiags;
 };
 
-TEST_F(HeadersTest, InsertInclude) {
-  std::string Path = testPath("sub/bar.h");
-  FS.Files[Path] = "";
-  EXPECT_EQ(calculate(Path), "\"bar.h\"");
-}
+MATCHER_P(Written, Name, "") { return arg.Written == Name; }
+MATCHER_P(Resolved, Name, "") { return arg.Resolved == Name; }
 
-TEST_F(HeadersTest, DontInsertDuplicateSameName) {
+TEST_F(HeadersTest, CollectR

[PATCH] D46497: [clangd] Populate #include insertions as additional edits in completion items.

[Eric Liu via Phabricator via cfe-commits]

ioeric added a comment.

This has been split and now depends on https://reviews.llvm.org/D46670, 
https://reviews.llvm.org/D46675, https://reviews.llvm.org/D46676.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D46497



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

[PATCH] D46676: [clangd] Remove LSP command-based #include insertion.

[Eric Liu via Phabricator via cfe-commits]

ioeric added a comment.

In https://reviews.llvm.org/D46676#1095713, @ilya-biryukov wrote:

> This LG, but we should first roll out the `additionalEdits` change.
>  Aren't the dependencies reversed in the dependency stack, i.e. this change 
> should be the last one?


You are right ;) I got the dependency reversed. My plan was to land this and 
the other patch at the "same" time.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D46676



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

[PATCH] D46670: [clangd] Move helpers that convert Replacements to TextEdits to SourceCode.h

[Eric Liu via Phabricator via cfe-commits]

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

- Got rid of a helper for std::vector


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D46670

Files:
  clangd/ClangdLSPServer.cpp
  clangd/SourceCode.cpp
  clangd/SourceCode.h


Index: clangd/SourceCode.h
===
--- clangd/SourceCode.h
+++ clangd/SourceCode.h
@@ -15,6 +15,7 @@
 #define LLVM_CLANG_TOOLS_EXTRA_CLANGD_SOURCECODE_H
 #include "Protocol.h"
 #include "clang/Basic/SourceLocation.h"
+#include "clang/Tooling/Core/Replacement.h"
 
 namespace clang {
 class SourceManager;
@@ -55,6 +56,11 @@
 std::pair
 splitQualifiedName(llvm::StringRef QName);
 
+TextEdit replacementToEdit(StringRef Code, const tooling::Replacement &R);
+
+std::vector replacementsToEdits(StringRef Code,
+  const tooling::Replacements &Repls);
+
 } // namespace clangd
 } // namespace clang
 #endif
Index: clangd/SourceCode.cpp
===
--- clangd/SourceCode.cpp
+++ clangd/SourceCode.cpp
@@ -166,5 +166,20 @@
   return {QName.substr(0, Pos + 2), QName.substr(Pos + 2)};
 }
 
+TextEdit replacementToEdit(StringRef Code, const tooling::Replacement &R) {
+  Range ReplacementRange = {
+  offsetToPosition(Code, R.getOffset()),
+  offsetToPosition(Code, R.getOffset() + R.getLength())};
+  return {ReplacementRange, R.getReplacementText()};
+}
+
+std::vector replacementsToEdits(StringRef Code,
+  const tooling::Replacements &Repls) {
+  std::vector Edits;
+  for (const auto &R : Repls)
+Edits.push_back(replacementToEdit(Code, R));
+  return Edits;
+}
+
 } // namespace clangd
 } // namespace clang
Index: clangd/ClangdLSPServer.cpp
===
--- clangd/ClangdLSPServer.cpp
+++ clangd/ClangdLSPServer.cpp
@@ -60,32 +60,6 @@
 static URISchemeRegistry::Add
 X("test", "Test scheme for clangd lit tests.");
 
-TextEdit replacementToEdit(StringRef Code, const tooling::Replacement &R) {
-  Range ReplacementRange = {
-  offsetToPosition(Code, R.getOffset()),
-  offsetToPosition(Code, R.getOffset() + R.getLength())};
-  return {ReplacementRange, R.getReplacementText()};
-}
-
-std::vector
-replacementsToEdits(StringRef Code,
-const std::vector &Replacements) {
-  // Turn the replacements into the format specified by the Language Server
-  // Protocol. Fuse them into one big JSON array.
-  std::vector Edits;
-  for (const auto &R : Replacements)
-Edits.push_back(replacementToEdit(Code, R));
-  return Edits;
-}
-
-std::vector replacementsToEdits(StringRef Code,
-  const tooling::Replacements &Repls) {
-  std::vector Edits;
-  for (const auto &R : Repls)
-Edits.push_back(replacementToEdit(Code, R));
-  return Edits;
-}
-
 SymbolKindBitset defaultSymbolKinds() {
   SymbolKindBitset Defaults;
   for (size_t I = SymbolKindMin; I <= static_cast(SymbolKind::Array);
@@ -291,7 +265,11 @@
   return replyError(ErrorCode::InternalError,
 llvm::toString(Replacements.takeError()));
 
-std::vector Edits = replacementsToEdits(*Code, 
*Replacements);
+// Turn the replacements into the format specified by the Language
+// Server Protocol. Fuse them into one big JSON array.
+std::vector Edits;
+for (const auto &R : *Replacements)
+  Edits.push_back(replacementToEdit(*Code, R));
 WorkspaceEdit WE;
 WE.changes = {{Params.textDocument.uri.uri(), Edits}};
 reply(WE);


Index: clangd/SourceCode.h
===
--- clangd/SourceCode.h
+++ clangd/SourceCode.h
@@ -15,6 +15,7 @@
 #define LLVM_CLANG_TOOLS_EXTRA_CLANGD_SOURCECODE_H
 #include "Protocol.h"
 #include "clang/Basic/SourceLocation.h"
+#include "clang/Tooling/Core/Replacement.h"
 
 namespace clang {
 class SourceManager;
@@ -55,6 +56,11 @@
 std::pair
 splitQualifiedName(llvm::StringRef QName);
 
+TextEdit replacementToEdit(StringRef Code, const tooling::Replacement &R);
+
+std::vector replacementsToEdits(StringRef Code,
+  const tooling::Replacements &Repls);
+
 } // namespace clangd
 } // namespace clang
 #endif
Index: clangd/SourceCode.cpp
===
--- clangd/SourceCode.cpp
+++ clangd/SourceCode.cpp
@@ -166,5 +166,20 @@
   return {QName.substr(0, Pos + 2), QName.substr(Pos + 2)};
 }
 
+TextEdit replacementToEdit(StringRef Code, const tooling::Replacement &R) {
+  Range ReplacementRange = {
+  offsetToPosition(Code, R.getOffset()),
+  offsetToPosition(Code, R.getOffset() + R.getLength())};
+  return {ReplacementRange, R.getReplacementText()};
+}
+
+std::vector replacementsToEdits(StringRef Code,
+  

[PATCH] D46670: [clangd] Move helpers that convert Replacements to TextEdits to SourceCode.h

[Eric Liu via Phabricator via cfe-commits]

ioeric added inline comments.



Comment at: clangd/SourceCode.h:62
+std::vector
+replacementsToEdits(StringRef Code,
+const std::vector &Replacements);

ilya-biryukov wrote:
> Do we really need to expose separate functions for 
> `vector` and `tooling::Replacements`, given that both 
> are trivial: loop over the array, convert each item?
> Having them in `.cpp` file didn't hurt, but they seems redundant in the 
> public header.
> WDYT?
Good point. Use of `std::vector` should be discouraged anyway. I 
removed this function and inline the conversion for `std::vector` 
as this is only used in rename (which should probably be fixed to use 
`tooling::Replacements` instead).


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D46670



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

[PATCH] D46670: [clangd] Move helpers that convert Replacements to TextEdits to SourceCode.h

[Eric Liu via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL332089: [clangd] Move helpers that convert Replacements to 
TextEdits to SourceCode.h (authored by ioeric, committed by ).
Herald added a subscriber: llvm-commits.

Repository:
  rL LLVM

https://reviews.llvm.org/D46670

Files:
  clang-tools-extra/trunk/clangd/ClangdLSPServer.cpp
  clang-tools-extra/trunk/clangd/SourceCode.cpp
  clang-tools-extra/trunk/clangd/SourceCode.h


Index: clang-tools-extra/trunk/clangd/SourceCode.cpp
===
--- clang-tools-extra/trunk/clangd/SourceCode.cpp
+++ clang-tools-extra/trunk/clangd/SourceCode.cpp
@@ -166,5 +166,20 @@
   return {QName.substr(0, Pos + 2), QName.substr(Pos + 2)};
 }
 
+TextEdit replacementToEdit(StringRef Code, const tooling::Replacement &R) {
+  Range ReplacementRange = {
+  offsetToPosition(Code, R.getOffset()),
+  offsetToPosition(Code, R.getOffset() + R.getLength())};
+  return {ReplacementRange, R.getReplacementText()};
+}
+
+std::vector replacementsToEdits(StringRef Code,
+  const tooling::Replacements &Repls) {
+  std::vector Edits;
+  for (const auto &R : Repls)
+Edits.push_back(replacementToEdit(Code, R));
+  return Edits;
+}
+
 } // namespace clangd
 } // namespace clang
Index: clang-tools-extra/trunk/clangd/SourceCode.h
===
--- clang-tools-extra/trunk/clangd/SourceCode.h
+++ clang-tools-extra/trunk/clangd/SourceCode.h
@@ -15,6 +15,7 @@
 #define LLVM_CLANG_TOOLS_EXTRA_CLANGD_SOURCECODE_H
 #include "Protocol.h"
 #include "clang/Basic/SourceLocation.h"
+#include "clang/Tooling/Core/Replacement.h"
 
 namespace clang {
 class SourceManager;
@@ -55,6 +56,11 @@
 std::pair
 splitQualifiedName(llvm::StringRef QName);
 
+TextEdit replacementToEdit(StringRef Code, const tooling::Replacement &R);
+
+std::vector replacementsToEdits(StringRef Code,
+  const tooling::Replacements &Repls);
+
 } // namespace clangd
 } // namespace clang
 #endif
Index: clang-tools-extra/trunk/clangd/ClangdLSPServer.cpp
===
--- clang-tools-extra/trunk/clangd/ClangdLSPServer.cpp
+++ clang-tools-extra/trunk/clangd/ClangdLSPServer.cpp
@@ -60,32 +60,6 @@
 static URISchemeRegistry::Add
 X("test", "Test scheme for clangd lit tests.");
 
-TextEdit replacementToEdit(StringRef Code, const tooling::Replacement &R) {
-  Range ReplacementRange = {
-  offsetToPosition(Code, R.getOffset()),
-  offsetToPosition(Code, R.getOffset() + R.getLength())};
-  return {ReplacementRange, R.getReplacementText()};
-}
-
-std::vector
-replacementsToEdits(StringRef Code,
-const std::vector &Replacements) {
-  // Turn the replacements into the format specified by the Language Server
-  // Protocol. Fuse them into one big JSON array.
-  std::vector Edits;
-  for (const auto &R : Replacements)
-Edits.push_back(replacementToEdit(Code, R));
-  return Edits;
-}
-
-std::vector replacementsToEdits(StringRef Code,
-  const tooling::Replacements &Repls) {
-  std::vector Edits;
-  for (const auto &R : Repls)
-Edits.push_back(replacementToEdit(Code, R));
-  return Edits;
-}
-
 SymbolKindBitset defaultSymbolKinds() {
   SymbolKindBitset Defaults;
   for (size_t I = SymbolKindMin; I <= static_cast(SymbolKind::Array);
@@ -291,7 +265,11 @@
   return replyError(ErrorCode::InternalError,
 llvm::toString(Replacements.takeError()));
 
-std::vector Edits = replacementsToEdits(*Code, 
*Replacements);
+// Turn the replacements into the format specified by the Language
+// Server Protocol. Fuse them into one big JSON array.
+std::vector Edits;
+for (const auto &R : *Replacements)
+  Edits.push_back(replacementToEdit(*Code, R));
 WorkspaceEdit WE;
 WE.changes = {{Params.textDocument.uri.uri(), Edits}};
 reply(WE);


Index: clang-tools-extra/trunk/clangd/SourceCode.cpp
===
--- clang-tools-extra/trunk/clangd/SourceCode.cpp
+++ clang-tools-extra/trunk/clangd/SourceCode.cpp
@@ -166,5 +166,20 @@
   return {QName.substr(0, Pos + 2), QName.substr(Pos + 2)};
 }
 
+TextEdit replacementToEdit(StringRef Code, const tooling::Replacement &R) {
+  Range ReplacementRange = {
+  offsetToPosition(Code, R.getOffset()),
+  offsetToPosition(Code, R.getOffset() + R.getLength())};
+  return {ReplacementRange, R.getReplacementText()};
+}
+
+std::vector replacementsToEdits(StringRef Code,
+  const tooling::Replacements &Repls) {
+  std::vector Edits;
+  for (const auto &R : Repls)
+Edits.push_back(replacementToEdit(Code, R));
+  return Edits;
+}
+
 } // namespace clangd
 } // namespace clang
Ind

[PATCH] D46751: [clangd] Filter out private proto symbols in SymbolCollector.

[Eric Liu via Phabricator via cfe-commits]

ioeric created this revision.
ioeric added a reviewer: ilya-biryukov.
Herald added subscribers: cfe-commits, jkorous, MaskRay, klimek.

This uses heuristics to identify private proto symbols. For example,
top-level symbols whose name contains "_" are considered private. These symbols
are not expected to be used by users.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D46751

Files:
  clangd/index/SymbolCollector.cpp
  unittests/clangd/SymbolCollectorTests.cpp


Index: unittests/clangd/SymbolCollectorTests.cpp
===
--- unittests/clangd/SymbolCollectorTests.cpp
+++ unittests/clangd/SymbolCollectorTests.cpp
@@ -697,6 +697,23 @@
AllOf(QName("pörk"), DeclRange(Header.range();
 }
 
+TEST_F(SymbolCollectorTest, FilterPrivateProtoSymbols) {
+  TestHeaderName = testPath("x.proto.h");
+  const std::string Header = R"(
+  namespace nx {
+class Top_Level {};
+class TopLevel {};
+enum Kind {
+  KIND_OK,
+  Kind_Not_Ok,
+};
+  }
+  )";
+  runSymbolCollector(Header, /*Main=*/"");
+  EXPECT_THAT(Symbols, UnorderedElementsAre(QName("nx"), QName("nx::TopLevel"),
+QName("nx::Kind"),
+QName("nx::KIND_OK")));
+}
 
 } // namespace
 } // namespace clangd
Index: clangd/index/SymbolCollector.cpp
===
--- clangd/index/SymbolCollector.cpp
+++ clangd/index/SymbolCollector.cpp
@@ -90,6 +90,28 @@
   return llvm::None;
 }
 
+bool isPrivateProtoSymbol(const NamedDecl *ND, const SourceManager &SM) {
+  auto FileName = SM.getFilename(findNameLoc(ND));
+  if (FileName.endswith(".proto.h") || FileName.endswith(".pb.h")) {
+auto Name = ND->getName();
+// Nested proto entities (e.g. Message::Nested) have top-level decls
+// that shouldn't be used (Message_Nested). Ignore them completely.
+// The nested entities are dangling type aliases, we may want to reconsider
+// including them in the future
+if (Name.contains('_')) {
+  if (ND->getKind() == Decl::EnumConstant) {
+// SOME_ENUM_CONSTANT is not private and should be indexed.
+// Outer_INNER is private.
+// This heuristic relies on naming style, it will include OUTER_INNER
+// and exclude some_enum_constant.
+return std::any_of(Name.begin(), Name.end(), islower);
+  }
+  return true;
+}
+  }
+  return false;
+}
+
 bool shouldFilterDecl(const NamedDecl *ND, ASTContext *ASTCtx,
   const SymbolCollector::Options &Opts) {
   using namespace clang::ast_matchers;
@@ -130,6 +152,9 @@
   .empty())
 return true;
 
+  if (isPrivateProtoSymbol(ND, ASTCtx->getSourceManager()))
+return true;
+
   return false;
 }
 


Index: unittests/clangd/SymbolCollectorTests.cpp
===
--- unittests/clangd/SymbolCollectorTests.cpp
+++ unittests/clangd/SymbolCollectorTests.cpp
@@ -697,6 +697,23 @@
AllOf(QName("pörk"), DeclRange(Header.range();
 }
 
+TEST_F(SymbolCollectorTest, FilterPrivateProtoSymbols) {
+  TestHeaderName = testPath("x.proto.h");
+  const std::string Header = R"(
+  namespace nx {
+class Top_Level {};
+class TopLevel {};
+enum Kind {
+  KIND_OK,
+  Kind_Not_Ok,
+};
+  }
+  )";
+  runSymbolCollector(Header, /*Main=*/"");
+  EXPECT_THAT(Symbols, UnorderedElementsAre(QName("nx"), QName("nx::TopLevel"),
+QName("nx::Kind"),
+QName("nx::KIND_OK")));
+}
 
 } // namespace
 } // namespace clangd
Index: clangd/index/SymbolCollector.cpp
===
--- clangd/index/SymbolCollector.cpp
+++ clangd/index/SymbolCollector.cpp
@@ -90,6 +90,28 @@
   return llvm::None;
 }
 
+bool isPrivateProtoSymbol(const NamedDecl *ND, const SourceManager &SM) {
+  auto FileName = SM.getFilename(findNameLoc(ND));
+  if (FileName.endswith(".proto.h") || FileName.endswith(".pb.h")) {
+auto Name = ND->getName();
+// Nested proto entities (e.g. Message::Nested) have top-level decls
+// that shouldn't be used (Message_Nested). Ignore them completely.
+// The nested entities are dangling type aliases, we may want to reconsider
+// including them in the future
+if (Name.contains('_')) {
+  if (ND->getKind() == Decl::EnumConstant) {
+// SOME_ENUM_CONSTANT is not private and should be indexed.
+// Outer_INNER is private.
+// This heuristic relies on naming style, it will include OUTER_INNER
+// and exclude some_enum_constant.
+return std::any_of(Name.begin(), Name.end(), islower);
+  }
+  return true;
+}
+  }
+  return false;
+}
+
 bool shouldFilterDecl(const NamedDecl *ND, ASTContext *ASTCtx,

[PATCH] D46676: [clangd] Remove LSP command-based #include insertion.

[Eric Liu via Phabricator via cfe-commits]

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



Comment at: unittests/clangd/ClangdTests.cpp:941
 
-TEST_F(ClangdVFSTest, InsertIncludes) {
-  MockFSProvider FS;

ilya-biryukov wrote:
> Do we test the same thing somewhere else  (e.g. code completion) in one of 
> the dependent changes?
> Maybe it's worth noting in the commit description that this test was not 
> removed completely, but instead moved to ?
Sure thing.

This is tested in code completion unit tests in D46497. Noted this in patch 
summary.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D46676



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

[PATCH] D46758: [clang-format] Move #include related style to libToolingCore

[Eric Liu via Phabricator via cfe-commits]

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

This will be shared by include insertion/deletion library.


Repository:
  rC Clang

https://reviews.llvm.org/D46758

Files:
  include/clang/Format/Format.h
  include/clang/Tooling/Core/IncludeStyle.h
  lib/Format/Format.cpp
  unittests/Format/FormatTest.cpp
  unittests/Format/SortIncludesTest.cpp

Index: unittests/Format/SortIncludesTest.cpp
===
--- unittests/Format/SortIncludesTest.cpp
+++ unittests/Format/SortIncludesTest.cpp
@@ -26,12 +26,12 @@
 
   std::string sort(StringRef Code, std::vector Ranges,
StringRef FileName = "input.cc") {
-auto Replaces = sortIncludes(Style, Code, Ranges, FileName);
+auto Replaces = sortIncludes(FmtStyle, Code, Ranges, FileName);
 Ranges = tooling::calculateRangesAfterReplacements(Replaces, Ranges);
 auto Sorted = applyAllReplacements(Code, Replaces);
 EXPECT_TRUE(static_cast(Sorted));
 auto Result = applyAllReplacements(
-*Sorted, reformat(Style, *Sorted, Ranges, FileName));
+*Sorted, reformat(FmtStyle, *Sorted, Ranges, FileName));
 EXPECT_TRUE(static_cast(Result));
 return *Result;
   }
@@ -41,12 +41,12 @@
   }
 
   unsigned newCursor(llvm::StringRef Code, unsigned Cursor) {
-sortIncludes(Style, Code, GetCodeRange(Code), "input.cpp", &Cursor);
+sortIncludes(FmtStyle, Code, GetCodeRange(Code), "input.cpp", &Cursor);
 return Cursor;
   }
 
-  FormatStyle Style = getLLVMStyle();
-
+  FormatStyle FmtStyle = getLLVMStyle();
+  tooling::IncludeStyle &Style = FmtStyle.IncludeStyle;
 };
 
 TEST_F(SortIncludesTest, BasicSorting) {
@@ -74,11 +74,11 @@
  "#include \n"
  "#include \n"
  "#include \n";
-  EXPECT_TRUE(sortIncludes(Style, Code, GetCodeRange(Code), "a.cc").empty());
+  EXPECT_TRUE(sortIncludes(FmtStyle, Code, GetCodeRange(Code), "a.cc").empty());
 }
 
 TEST_F(SortIncludesTest, SortedIncludesInMultipleBlocksAreMerged) {
-  Style.IncludeBlocks = FormatStyle::IBS_Merge;
+  Style.IncludeBlocks = tooling::IncludeStyle::IBS_Merge;
   EXPECT_EQ("#include \"a.h\"\n"
 "#include \"b.h\"\n"
 "#include \"c.h\"\n",
@@ -88,7 +88,7 @@
  "\n"
  "#include \"b.h\"\n"));
 
-  Style.IncludeBlocks = FormatStyle::IBS_Regroup;
+  Style.IncludeBlocks = tooling::IncludeStyle::IBS_Regroup;
   EXPECT_EQ("#include \"a.h\"\n"
 "#include \"b.h\"\n"
 "#include \"c.h\"\n",
@@ -119,7 +119,7 @@
 }
 
 TEST_F(SortIncludesTest, IncludeSortingCanBeDisabled) {
-  Style.SortIncludes = false;
+  FmtStyle.SortIncludes = false;
   EXPECT_EQ("#include \"a.h\"\n"
 "#include \"c.h\"\n"
 "#include \"b.h\"\n",
@@ -182,7 +182,7 @@
 }
 
 TEST_F(SortIncludesTest, SortsAllBlocksWhenMerging) {
-  Style.IncludeBlocks = FormatStyle::IBS_Merge;
+  Style.IncludeBlocks = tooling::IncludeStyle::IBS_Merge;
   EXPECT_EQ("#include \"a.h\"\n"
 "#include \"b.h\"\n"
 "#include \"c.h\"\n",
@@ -202,7 +202,7 @@
  "// comment\n"
  "#include \"b.h\"\n"));
 
-  Style.IncludeBlocks = FormatStyle::IBS_Merge;
+  Style.IncludeBlocks = tooling::IncludeStyle::IBS_Merge;
   EXPECT_EQ("#include \"a.h\"\n"
 "#include \"c.h\"\n"
 "// comment\n"
@@ -212,7 +212,7 @@
  "// comment\n"
  "#include \"b.h\"\n"));
 
-  Style.IncludeBlocks = FormatStyle::IBS_Regroup;
+  Style.IncludeBlocks = tooling::IncludeStyle::IBS_Regroup;
   EXPECT_EQ("#include \"a.h\"\n"
 "#include \"c.h\"\n"
 "// comment\n"
@@ -233,7 +233,7 @@
  "#include \"c.h\"\n"
  "#include \"a.h\"\n"));
 
-  Style = getGoogleStyle(FormatStyle::LK_Cpp);
+  FmtStyle = getGoogleStyle(FormatStyle::LK_Cpp);
   EXPECT_EQ("#include \n"
 "#include \n"
 "#include \"a.h\"\n"
@@ -245,7 +245,7 @@
 }
 
 TEST_F(SortIncludesTest, RegroupsAngledIncludesInSeparateBlocks) {
-  Style.IncludeBlocks = FormatStyle::IBS_Regroup;
+  Style.IncludeBlocks = tooling::IncludeStyle::IBS_Regroup;
   EXPECT_EQ("#include \"a.h\"\n"
 "#include \"c.h\"\n"
 "\n"
@@ -345,7 +345,7 @@
 
 TEST_F(SortIncludesTest, RecognizeMainHeaderInAllGroups) {
   Style.IncludeIsMainRegex = "([-_](test|unittest))?$";
-  Style.IncludeBlocks = FormatStyle::IBS_Merge;
+  Style.IncludeBlocks = tooling::IncludeStyle::IBS_Merge;
 
   EXPECT_EQ("#include \"c.h\"\n"
 "#include \"a.h\"\n"
@@ -359,7 +359,7 @@
 
 TEST_F(SortIncludesTest, MainHeaderIsSeparatedWhenRegroupping) {
   Style.IncludeIsMainRegex = "([-_](test|unittest))?$";
-  Style.IncludeBlocks = FormatStyle::IBS_Regroup;
+  Style.IncludeBlocks = tooling::IncludeStyle::IBS_Regroup;
 
   EXPECT_EQ("#include \"a.h\"\n"
 "\n"
@@ -417,7 +417,7 @@
 
 

[PATCH] D46751: [clangd] Filter out private proto symbols in SymbolCollector.

[Eric Liu via Phabricator via cfe-commits]

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

- Addressed a few comments.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D46751

Files:
  clangd/index/SymbolCollector.cpp
  unittests/clangd/SymbolCollectorTests.cpp


Index: unittests/clangd/SymbolCollectorTests.cpp
===
--- unittests/clangd/SymbolCollectorTests.cpp
+++ unittests/clangd/SymbolCollectorTests.cpp
@@ -697,6 +697,39 @@
AllOf(QName("pörk"), DeclRange(Header.range();
 }
 
+TEST_F(SymbolCollectorTest, FilterPrivateProtoSymbols) {
+  TestHeaderName = testPath("x.proto.h");
+  const std::string Header = R"(
+  namespace nx {
+class Top_Level {};
+class TopLevel {};
+enum Kind {
+  KIND_OK,
+  Kind_Not_Ok,
+};
+  }
+  )";
+  runSymbolCollector(Header, /*Main=*/"");
+  EXPECT_THAT(Symbols, UnorderedElementsAre(QName("nx"), QName("nx::TopLevel"),
+QName("nx::Kind"),
+QName("nx::KIND_OK")));
+}
+
+TEST_F(SymbolCollectorTest, DontFilterNonProtoSymbols) {
+  TestHeaderName = testPath("x.h");
+  const std::string Header = R"(
+  namespace nx {
+class Top_Level {};
+enum Kind {
+  Kind_Fine
+};
+  }
+  )";
+  runSymbolCollector(Header, /*Main=*/"");
+  EXPECT_THAT(Symbols,
+  UnorderedElementsAre(QName("nx"), QName("nx::Top_Level"),
+   QName("nx::Kind"), QName("nx::Kind_Fine")));
+}
 
 } // namespace
 } // namespace clangd
Index: clangd/index/SymbolCollector.cpp
===
--- clangd/index/SymbolCollector.cpp
+++ clangd/index/SymbolCollector.cpp
@@ -90,6 +90,24 @@
   return llvm::None;
 }
 
+bool isPrivateProtoSymbol(const NamedDecl *ND, const SourceManager &SM) {
+  auto FileName = SM.getFilename(findNameLoc(ND));
+  if (!FileName.endswith(".proto.h") && !FileName.endswith(".pb.h"))
+return false;
+  auto Name = ND->getName();
+  if (!Name.contains('_'))
+return false;
+  // Nested proto entities (e.g. Message::Nested) have top-level decls
+  // that shouldn't be used (Message_Nested). Ignore them completely.
+  // The nested entities are dangling type aliases, we may want to reconsider
+  // including them in the future.
+  // For enum constants, SOME_ENUM_CONSTANT is not private and should be
+  // indexed. Outer_INNER is private. This heuristic relies on naming style, it
+  // will include OUTER_INNER and exclude some_enum_constant.
+  return (ND->getKind() != Decl::EnumConstant) ||
+ std::any_of(Name.begin(), Name.end(), islower);
+}
+
 bool shouldFilterDecl(const NamedDecl *ND, ASTContext *ASTCtx,
   const SymbolCollector::Options &Opts) {
   using namespace clang::ast_matchers;
@@ -130,6 +148,10 @@
   .empty())
 return true;
 
+  // Avoid indexing internal symbols in protobuf generated headers.
+  if (isPrivateProtoSymbol(ND, ASTCtx->getSourceManager()))
+return true;
+
   return false;
 }
 


Index: unittests/clangd/SymbolCollectorTests.cpp
===
--- unittests/clangd/SymbolCollectorTests.cpp
+++ unittests/clangd/SymbolCollectorTests.cpp
@@ -697,6 +697,39 @@
AllOf(QName("pörk"), DeclRange(Header.range();
 }
 
+TEST_F(SymbolCollectorTest, FilterPrivateProtoSymbols) {
+  TestHeaderName = testPath("x.proto.h");
+  const std::string Header = R"(
+  namespace nx {
+class Top_Level {};
+class TopLevel {};
+enum Kind {
+  KIND_OK,
+  Kind_Not_Ok,
+};
+  }
+  )";
+  runSymbolCollector(Header, /*Main=*/"");
+  EXPECT_THAT(Symbols, UnorderedElementsAre(QName("nx"), QName("nx::TopLevel"),
+QName("nx::Kind"),
+QName("nx::KIND_OK")));
+}
+
+TEST_F(SymbolCollectorTest, DontFilterNonProtoSymbols) {
+  TestHeaderName = testPath("x.h");
+  const std::string Header = R"(
+  namespace nx {
+class Top_Level {};
+enum Kind {
+  Kind_Fine
+};
+  }
+  )";
+  runSymbolCollector(Header, /*Main=*/"");
+  EXPECT_THAT(Symbols,
+  UnorderedElementsAre(QName("nx"), QName("nx::Top_Level"),
+   QName("nx::Kind"), QName("nx::Kind_Fine")));
+}
 
 } // namespace
 } // namespace clangd
Index: clangd/index/SymbolCollector.cpp
===
--- clangd/index/SymbolCollector.cpp
+++ clangd/index/SymbolCollector.cpp
@@ -90,6 +90,24 @@
   return llvm::None;
 }
 
+bool isPrivateProtoSymbol(const NamedDecl *ND, const SourceManager &SM) {
+  auto FileName = SM.getFilename(findNameLoc(ND));
+  if (!FileName.endswith(".proto.h") && !FileName.endswith(".pb.h"))
+return false;
+  auto Name = ND->getName();
+  if (!Name.contains('

[PATCH] D46751: [clangd] Filter out private proto symbols in SymbolCollector.

[Eric Liu via Phabricator via cfe-commits]

ioeric added a comment.

Thanks for sharing the example Marc! It's a bit surprising to see files that 
are not protobuf-generated named proto.h.

I'm not a big fan of parsing file comment in proto. It seems a bit cumbersome 
and we might not be able (or too expensive) to do so for completion results 
from sema (if we do want to filter at completion time).

Pattern-based filtering could be an option as it wouldn't require code 
modification and could support potentially more filters, although I'm a bit 
worries about rules getting too complicated (e.g. filters on symbol kinds etc) 
or running into limitation.

But for now, it seems to me that the easiest approach is putting an option 
around proto heuristic for the symbol collector, until we need more filters.




Comment at: clangd/index/SymbolCollector.cpp:156
+  if (isPrivateProtoSymbol(ND, ASTCtx->getSourceManager()))
+return true;
+

ilya-biryukov wrote:
> We should also run the same code to filter our those decls coming from sema 
> completions.
> Otherwise, they might still pop up in completion results if internal proto 
> symbols were deserialized from preamble by clang before running the code 
> completion.
Just want to clarify before going further. IIUC, in *index-based* completion, 
the preamble could still contain symbols from headers such that sema completion 
could still give you symbols from headers. 

If we do need to build the filter into code completion, we would need more 
careful design as code completion code path is more latency sensitive.



Comment at: unittests/clangd/SymbolCollectorTests.cpp:713
+  runSymbolCollector(Header, /*Main=*/"");
+  EXPECT_THAT(Symbols, UnorderedElementsAre(QName("nx"), QName("nx::TopLevel"),
+QName("nx::Kind"),

ilya-biryukov wrote:
> Maybe also test that the same symbol is not excluded in the  file that does 
> not end with `.proto.h`?
Sounds good.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D46751



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