[clangd] Penalize destructor and overloaded operators in code completion.

Reviewers: hokein

Subscribers: ilya-biryukov, MaskRay, jkorous, arphaman, kadircet, cfe-commits

Differential Revision: https://reviews.llvm.org/D55061

llvm-svn: 347983

3 files changed, 55 insertions, 10 deletions

diff --git a/clang-tools-extra/clangd/Quality.cpp b/clang-tools-extra/clangd/Quality.cpp
index 1e9211b9cb1..fe7a969eb3a 100644
--- a/clang-tools-extra/clangd/Quality.cpp
+++ b/clang-tools-extra/clangd/Quality.cpp
@@ -62,6 +62,10 @@ static bool hasUsingDeclInMainFile(const CodeCompletionResult &R) {
 }
 
 static SymbolQualitySignals::SymbolCategory categorize(const NamedDecl &ND) {
+  if (const auto *FD = dyn_cast<FunctionDecl>(&ND)) {
+    if (FD->isOverloadedOperator())
+      return SymbolQualitySignals::Operator;
+  }
   class Switch
       : public ConstDeclVisitor<Switch, SymbolQualitySignals::SymbolCategory> {
   public:
@@ -75,6 +79,7 @@ static SymbolQualitySignals::SymbolCategory categorize(const NamedDecl &ND) {
     MAP(TypeAliasTemplateDecl, Type);
     MAP(ClassTemplateDecl, Type);
     MAP(CXXConstructorDecl, Constructor);
+    MAP(CXXDestructorDecl, Destructor);
     MAP(ValueDecl, Variable);
     MAP(VarTemplateDecl, Variable);
     MAP(FunctionDecl, Function);
@@ -134,9 +139,10 @@ categorize(const index::SymbolInfo &D) {
   case index::SymbolKind::InstanceProperty:
   case index::SymbolKind::ClassProperty:
   case index::SymbolKind::StaticProperty:
-  case index::SymbolKind::Destructor:
   case index::SymbolKind::ConversionFunction:
     return SymbolQualitySignals::Function;
+  case index::SymbolKind::Destructor:
+    return SymbolQualitySignals::Destructor;
   case index::SymbolKind::Constructor:
     return SymbolQualitySignals::Constructor;
   case index::SymbolKind::Variable:
@@ -231,10 +237,12 @@ float SymbolQualitySignals::evaluate() const {
     Score *= 0.8f;
     break;
   case Macro:
+  case Destructor:
+  case Operator:
     Score *= 0.5f;
     break;
-  case Unknown:
   case Constructor: // No boost constructors so they are after class types.
+  case Unknown:
     break;
   }
 
diff --git a/clang-tools-extra/clangd/Quality.h b/clang-tools-extra/clangd/Quality.h
index f85e427fbe1..9cb0c2f3085 100644
--- a/clang-tools-extra/clangd/Quality.h
+++ b/clang-tools-extra/clangd/Quality.h
@@ -68,8 +68,10 @@ struct SymbolQualitySignals {
     Type,
     Function,
     Constructor,
+    Destructor,
     Namespace,
     Keyword,
+    Operator,
   } Category = Unknown;
 
   void merge(const CodeCompletionResult &SemaCCResult);
diff --git a/clang-tools-extra/unittests/clangd/QualityTests.cpp b/clang-tools-extra/unittests/clangd/QualityTests.cpp
index cc330f28a52..f1a1fdae6b4 100644
--- a/clang-tools-extra/unittests/clangd/QualityTests.cpp
+++ b/clang-tools-extra/unittests/clangd/QualityTests.cpp
@@ -205,16 +205,22 @@ TEST(QualityTests, SymbolQualitySignalsSanity) {
   EXPECT_GT(WithReferences.evaluate(), Default.evaluate());
   EXPECT_GT(ManyReferences.evaluate(), WithReferences.evaluate());
 
-  SymbolQualitySignals Keyword, Variable, Macro, Constructor, Function;
+  SymbolQualitySignals Keyword, Variable, Macro, Constructor, Function,
+      Destructor, Operator;
   Keyword.Category = SymbolQualitySignals::Keyword;
   Variable.Category = SymbolQualitySignals::Variable;
   Macro.Category = SymbolQualitySignals::Macro;
   Constructor.Category = SymbolQualitySignals::Constructor;
+  Destructor.Category = SymbolQualitySignals::Destructor;
+  Destructor.Category = SymbolQualitySignals::Destructor;
+  Operator.Category = SymbolQualitySignals::Operator;
   Function.Category = SymbolQualitySignals::Function;
   EXPECT_GT(Variable.evaluate(), Default.evaluate());
   EXPECT_GT(Keyword.evaluate(), Variable.evaluate());
   EXPECT_LT(Macro.evaluate(), Default.evaluate());
+  EXPECT_LT(Operator.evaluate(), Default.evaluate());
   EXPECT_LT(Constructor.evaluate(), Function.evaluate());
+  EXPECT_LT(Destructor.evaluate(), Constructor.evaluate());
 }
 
 TEST(QualityTests, SymbolRelevanceSignalsSanity) {
@@ -385,11 +391,12 @@ TEST(QualityTests, IsInstanceMember) {
   EXPECT_TRUE(Rel.IsInstanceMember);
 }
 
-TEST(QualityTests, ConstructorQuality) {
+TEST(QualityTests, ConstructorDestructor) {
   auto Header = TestTU::withHeaderCode(R"cpp(
     class Foo {
     public:
       Foo(int);
+      ~Foo();
     };
   )cpp");
   auto Symbols = Header.headerSymbols();
@@ -399,15 +406,43 @@ TEST(QualityTests, ConstructorQuality) {
     return (ND.getQualifiedNameAsString() == "Foo::Foo") &&
            isa<CXXConstructorDecl>(&ND);
   });
+  const NamedDecl *DtorDecl = &findDecl(AST, [](const NamedDecl &ND) {
+    return (ND.getQualifiedNameAsString() == "Foo::~Foo") &&
+           isa<CXXDestructorDecl>(&ND);
+  });
 
-  SymbolQualitySignals Q;
-  Q.merge(CodeCompletionResult(CtorDecl, /*Priority=*/0));
-  EXPECT_EQ(Q.Category, SymbolQualitySignals::Constructor);
+  SymbolQualitySignals CtorQ;
+  CtorQ.merge(CodeCompletionResult(CtorDecl, /*Priority=*/0));
+  EXPECT_EQ(CtorQ.Category, SymbolQualitySignals::Constructor);
 
-  Q.Category = SymbolQualitySignals::Unknown;
+  CtorQ.Category = SymbolQualitySignals::Unknown;
   const Symbol &CtorSym = findSymbol(Symbols, "Foo::Foo");
-  Q.merge(CtorSym);
-  EXPECT_EQ(Q.Category, SymbolQualitySignals::Constructor);
+  CtorQ.merge(CtorSym);
+  EXPECT_EQ(CtorQ.Category, SymbolQualitySignals::Constructor);
+
+  SymbolQualitySignals DtorQ;
+  DtorQ.merge(CodeCompletionResult(DtorDecl, /*Priority=*/0));
+  EXPECT_EQ(DtorQ.Category, SymbolQualitySignals::Destructor);
+}
+
+TEST(QualityTests, Operator) {
+  auto Header = TestTU::withHeaderCode(R"cpp(
+    class Foo {
+    public:
+      bool operator<(const Foo& f1, const Foo& f2);
+    };
+  )cpp");
+  auto AST = Header.build();
+
+  const NamedDecl *Operator = &findDecl(AST, [](const NamedDecl &ND) {
+    if (const auto *OD = dyn_cast<FunctionDecl>(&ND))
+      if (OD->isOverloadedOperator())
+        return true;
+    return false;
+  });
+  SymbolQualitySignals Q;
+  Q.merge(CodeCompletionResult(Operator, /*Priority=*/0));
+  EXPECT_EQ(Q.Category, SymbolQualitySignals::Operator);
 }
 
 TEST(QualityTests, ItemWithFixItsRankedDown) {