[PATCH] D53131: [clangd] Support scope proximity in code completion.

[Sam McCall via Phabricator via cfe-commits]

sammccall added inline comments.


================
Comment at: clangd/CodeComplete.cpp:558
+    if (const auto *NS = dyn_cast<NamespaceDecl>(Ctx))
+      return NS->getQualifiedNameAsString() + "::";
+  return llvm::None;
----------------
does this do the right thing if it's anonymous or inline, or a parent is?

For indexing, we call a function in AST.h, we should probably do something similar.

The catch is that function relies on NamedDecl::printQualifiedName, maybe we need to factor out the relevant part so we can call it on a DeclContext.


================
Comment at: clangd/CodeComplete.cpp:559
+      return NS->getQualifiedNameAsString() + "::";
+  return llvm::None;
+}
----------------
shouldn't this be ""? That's a definite scope, not a failure to find one.
(It's not a *namespace* scope, but I'm not sure why that's important)


================
Comment at: clangd/Quality.cpp:246
 
+float QueryScopeProximity::proximity(llvm::StringRef Scope) const {
+  if (QueryScopes.empty())
----------------
If you're going to decide these numbers directly...

a) I think you should just return a multiplier here, rather than a 0-1 score
b) we should more aggressively downrank non-preferred symbols: currently by only ~1/3 vs symbols from a preferred scopes

e.g. I'd suggest returning 1, 2, 1.5, 1, 1, 1.5, 0.3 or similar


================
Comment at: clangd/Quality.cpp:301
+    // results can be tricky (e.g. class/function scope), and sema results are
+    // always in the accessible scope.
+    SemaScopeProximityScore = 1.0;
----------------
I don't think "always in the accessible scope" is sufficient justification to give a score of 1.
However "we don't load top-level symbols from the preamble" might be?


================
Comment at: clangd/Quality.h:92
+  private:
+    std::vector<std::string> QueryScopes;
+};
----------------
hmm, can/should we use FileProximity for this, and just transform the strings?

This isn't going to cache for symbols sharing a namespace, isn't going to handle "symbol is in a child namespace of an included namespace", and some other combinations.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D53131