[PATCH] D47762: [clangd] Boost code completion results that are narrowly scoped (local, members)

[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/Quality.cpp:70
+    return SymbolRelevanceSignals::ClassScope;
+  if (D.getLinkageInternal() < ExternalLinkage)
+    return SymbolRelevanceSignals::FileScope;
----------------
sammccall wrote:
> ioeric wrote:
> > I wonder if this comparison is a common practice. Any reason not to use `!=`?
> The orderedness in linkage is used e.g. in `isExternallyVisible()`.
> I prefer `<` because while with this particular threshold value `!=` would be equivalent, I'm not actually particularly sure it's the right threshold (vs e.g. `ModuleLinkage`), and with any other threshold `<` would be required.
sounds good. this would probably deserve a comment though as it's a little confusing...


================
Comment at: clangd/Quality.cpp:71
+  if (D.getLinkageInternal() < ExternalLinkage)
+    return SymbolRelevanceSignals::FileScope;
+  return SymbolRelevanceSignals::GlobalScope;
----------------
sammccall wrote:
> ioeric wrote:
> > Does `FileScope` indicate that `D` is defined in the current *translation unit*? If so, I think this is a good signal (boosting symbols in the same TU), but something like `TUScope` might be a bit clearer?
> File rather than TU is intended here. `AccessibleScope` is only an approximate concept (it doesn't need to be precise as it's just a scoring signal).
> The idea is "this is a symbol likely to be only used in the same file that declares it", and this is true of e.g. variables in anonymous namespaces, despite the fact that you can technically put them in headers and reference them in the main file.
> 
> Added a comment to AccessibleScope to clarify this approximate nature.
Cool. Thanks for the explanation!


================
Comment at: unittests/clangd/QualityTests.cpp:1
 //===-- SourceCodeTests.cpp  ------------------------------------*- C++ -*-===//
 //
----------------
sammccall wrote:
> ioeric wrote:
> > Could you also add a test case for code completion?
> The code completion scoring is tested in SymbolRelevanceSignalsSanity: file scope is boosted compared to default when the query is code-complete, but not when it's generic.
> 
> What kind of test do you mean?
I was thinking a test case that covers the changes in CodeComplete.cpp e.g. check that Relevance and Quality play well together, and locals/members are boosted? Would that make sense?


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D47762