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

[Eric Liu via Phabricator via cfe-commits]

ioeric added a comment.

Looks like a great improvement on ranking!



================
Comment at: clangd/Quality.cpp:62
+  bool InClass;
+  for (const DeclContext *DC = D.getDeclContext(); DC != nullptr;
+       DC = DC->getParent()) {
----------------
nit: maybe stop when`!DC->isFileContext()`?


================
Comment at: clangd/Quality.cpp:66
+      return SymbolRelevanceSignals::FunctionScope;
+    InClass = InClass || isa<RecordDecl>(DC);
+  }
----------------
`DC->isRecord()`?


================
Comment at: clangd/Quality.cpp:70
+    return SymbolRelevanceSignals::ClassScope;
+  if (D.getLinkageInternal() < ExternalLinkage)
+    return SymbolRelevanceSignals::FileScope;
----------------
I wonder if this comparison is a common practice. Any reason not to use `!=`?


================
Comment at: clangd/Quality.cpp:71
+  if (D.getLinkageInternal() < ExternalLinkage)
+    return SymbolRelevanceSignals::FileScope;
+  return SymbolRelevanceSignals::GlobalScope;
----------------
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?


================
Comment at: clangd/Quality.cpp:76
+void SymbolRelevanceSignals::merge(const Symbol &IndexResult) {
+  // FIXME: Index results always assumed to be at global scope.
+}
----------------
What's here to be fixed? Do we plan to get rid of this assumption in the future?


================
Comment at: clangd/Quality.cpp:86
+  if (SemaCCResult.Kind == CodeCompletionResult::RK_Declaration)
+    Scope = std::min(Scope, ComputeScope(*SemaCCResult.Declaration));
 }
----------------
nit: does this assume that "smaller" scopes are better? If so, it might make sense to document.


================
Comment at: unittests/clangd/QualityTests.cpp:1
 //===-- SourceCodeTests.cpp  ------------------------------------*- C++ -*-===//
 //
----------------
Could you also add a test case for code completion?


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D47762