[PATCH] D42073: [clangd] Use accessible scopes to query indexes for global code completion.

[Sam McCall via Phabricator via cfe-commits]

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


================
Comment at: clangd/CodeComplete.cpp:868
+    }
+    log(Ctx, "Query scopes: [");
+    for (auto &R : Req.Scopes)
----------------
hokein wrote:
> sammccall wrote:
> > log doesn't work that way - you need to combine into a single message :-)
> > 
> > I think since we don't have verbosity levels, it's best to remove this - it's much more fine-grained than anything else we log, so it will be spammy.
> > (If we do want to log something, we should summarize the whole query.)
> I think the log here is pretty useful for debugging. 
SG. It might be worth more clearly tying the log message to the other code complete one:, e.g. "Code complete: fuzzyFind(\"{0}\", Scopes=[{1}])"


================
Comment at: clangd/CodeComplete.cpp:652
 
-SpecifiedScope getSpecifiedScope(Sema &S, const CXXScopeSpec &SS) {
-  SpecifiedScope Info;
-  auto &SM = S.getSourceManager();
-  auto SpecifierRange = SS.getRange();
-  Info.Written = Lexer::getSourceText(
-      CharSourceRange::getCharRange(SpecifierRange), SM, clang::LangOptions());
-  if (!Info.Written.empty())
-    Info.Written += "::"; // Sema excludes the trailing ::.
-  if (SS.isValid()) {
-    DeclContext *DC = S.computeDeclContext(SS);
-    if (auto *NS = llvm::dyn_cast<NamespaceDecl>(DC)) {
-      Info.Resolved = NS->getQualifiedNameAsString() + "::";
-    } else if (llvm::dyn_cast<TranslationUnitDecl>(DC) != nullptr) {
-      Info.Resolved = "";
+// Get all scopes that will be queried in indexes.
+std::vector<std::string> getQueryScopes(Sema &S,
----------------
could you move this to be right after the closely-related SpecifiedScope struct? I can't remember why they got separated, probably my fault :-)


================
Comment at: clangd/CodeComplete.cpp:653
+// Get all scopes that will be queried in indexes.
+std::vector<std::string> getQueryScopes(Sema &S,
+                                        CodeCompletionContext &CCContext) {
----------------
as Ilya pointed out to me, passing Sema around is a big scary thing that we should try to avoid. Here it's only used to get the text if the CXXScopeSpec is invalid. Can we pass just the SourceManager, or better yet, the text as a StringPiece?


================
Comment at: clangd/CodeComplete.cpp:695
+  Info.UnresolvedQualifier =
+      Lexer::getSourceText(CharSourceRange::getCharRange((*SS)->getRange()),
+                           S.getSourceManager(), clang::LangOptions());
----------------
do you need to remove any leading :: here?


================
Comment at: unittests/clangd/CodeCompleteTests.cpp:679
 }
 
+class IndexRequestCollector : public SymbolIndex {
----------------
Can you factor out the common code into a function?

e.g. 
  vector<FuzzyFindRequest> captureIndexRequests(string example) {
    IndexRequestCollector Index; // actually the class can be local here
    ...
  }

Like completions(), this avoids boilerplate in the tests.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D42073