[PATCH] D48368: [clangd] Sema ranking tweaks: downrank keywords and injected names.

[Sam McCall via Phabricator via cfe-commits]

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

Injected names being ranked too high was just a bug.
The high boost for keywords was intended, but was too much given how useless
keywords are. We should probably boost them on a case-by-case basis eventually.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D48368

Files:
  clangd/Quality.cpp
  unittests/clangd/QualityTests.cpp


Index: unittests/clangd/QualityTests.cpp
===================================================================
--- unittests/clangd/QualityTests.cpp
+++ unittests/clangd/QualityTests.cpp
@@ -84,6 +84,7 @@
     int deprecated() { return 0; }
 
     namespace { struct X { void y() { int z; } }; }
+    struct S{}
   )cpp";
   auto AST = Test.build();
 
@@ -115,6 +116,10 @@
   Relevance = {};
   Relevance.merge(CodeCompletionResult(&findAnyDecl(AST, "z"), 42));
   EXPECT_EQ(Relevance.Scope, SymbolRelevanceSignals::FunctionScope);
+  // The injected class name is treated as the outer class name.
+  Relevance = {};
+  Relevance.merge(CodeCompletionResult(&findDecl(AST, "S::S"), 42));
+  EXPECT_EQ(Relevance.Scope, SymbolRelevanceSignals::GlobalScope);
 }
 
 // Do the signals move the scores in the direction we expect?
Index: clangd/Quality.cpp
===================================================================
--- clangd/Quality.cpp
+++ clangd/Quality.cpp
@@ -156,8 +156,8 @@
     Score *= 0.1f;
 
   switch (Category) {
-    case Keyword:  // Usually relevant, but misses most signals.
-      Score *= 10;
+    case Keyword:  // Often relevant, but misses most signals.
+      Score *= 4;  // FIXME: important keywords should have specific boosts.
       break;
     case Type:
     case Function:
@@ -241,18 +241,22 @@
 }
 
 static SymbolRelevanceSignals::AccessibleScope
-ComputeScope(const NamedDecl &D) {
+ComputeScope(const NamedDecl *D) {
+  // Injected "Foo" within the class "Foo" has file scope, not class scope.
+  const DeclContext *DC = D->getDeclContext();
+  if (auto *R = dyn_cast_or_null<RecordDecl>(D))
+    if (R->isInjectedClassName())
+      DC = DC->getParent();
   bool InClass = false;
-  for (const DeclContext *DC = D.getDeclContext(); !DC->isFileContext();
-       DC = DC->getParent()) {
+  for (; !DC->isFileContext(); DC = DC->getParent()) {
     if (DC->isFunctionOrMethod())
       return SymbolRelevanceSignals::FunctionScope;
     InClass = InClass || DC->isRecord();
   }
   if (InClass)
     return SymbolRelevanceSignals::ClassScope;
   // This threshold could be tweaked, e.g. to treat module-visible as global.
-  if (D.getLinkageInternal() < ExternalLinkage)
+  if (D->getLinkageInternal() < ExternalLinkage)
     return SymbolRelevanceSignals::FileScope;
   return SymbolRelevanceSignals::GlobalScope;
 }
@@ -280,7 +284,7 @@
 
   // Declarations are scoped, others (like macros) are assumed global.
   if (SemaCCResult.Declaration)
-    Scope = std::min(Scope, ComputeScope(*SemaCCResult.Declaration));
+    Scope = std::min(Scope, ComputeScope(SemaCCResult.Declaration));
 }
 
 float SymbolRelevanceSignals::evaluate() const {


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D48368.152088.patch
Type: text/x-patch
Size: 2674 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20180620/42086daa/attachment.bin>