[clang-tools-extra] 7d1b499 - Revert "[clangd] Extract symbol-scope logic out of Quality, add tests. NFC"
Sam McCall via cfe-commits
cfe-commits at lists.llvm.org
Fri Jan 29 06:01:54 PST 2021
Author: Sam McCall
Date: 2021-01-29T14:59:16+01:00
New Revision: 7d1b499caef6ebde09a2697a97b43b89f7fa35c8
URL: https://github.com/llvm/llvm-project/commit/7d1b499caef6ebde09a2697a97b43b89f7fa35c8
DIFF: https://github.com/llvm/llvm-project/commit/7d1b499caef6ebde09a2697a97b43b89f7fa35c8.diff
LOG: Revert "[clangd] Extract symbol-scope logic out of Quality, add tests. NFC"
On second thought, this can't properly be reused for highlighting.
Consider this example, which Quality wants to consider function-scope,
but highlighting must consider class-scope:
void foo() {
class X {
int ^y;
};
}
Added:
Modified:
clang-tools-extra/clangd/AST.cpp
clang-tools-extra/clangd/AST.h
clang-tools-extra/clangd/CodeComplete.cpp
clang-tools-extra/clangd/Quality.cpp
clang-tools-extra/clangd/Quality.h
clang-tools-extra/clangd/quality/CompletionModelCodegen.py
clang-tools-extra/clangd/quality/model/features.json
clang-tools-extra/clangd/unittests/ASTTests.cpp
clang-tools-extra/clangd/unittests/QualityTests.cpp
Removed:
################################################################################
diff --git a/clang-tools-extra/clangd/AST.cpp b/clang-tools-extra/clangd/AST.cpp
index f71d935c204c..aecaf7e6b8f7 100644
--- a/clang-tools-extra/clangd/AST.cpp
+++ b/clang-tools-extra/clangd/AST.cpp
@@ -478,29 +478,5 @@ bool hasUnstableLinkage(const Decl *D) {
return VD && !VD->getType().isNull() && VD->getType()->isUndeducedType();
}
-SymbolScope symbolScope(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<RecordDecl>(&D))
- if (R->isInjectedClassName())
- DC = DC->getParent();
- // Class constructor should have the same scope as the class.
- if (isa<CXXConstructorDecl>(D))
- DC = DC->getParent();
- bool InClass = false;
- for (; !DC->isFileContext(); DC = DC->getParent()) {
- if (DC->isFunctionOrMethod())
- return SymbolScope::FunctionScope;
- InClass = InClass || DC->isRecord();
- }
- if (InClass)
- return SymbolScope::ClassScope;
- // ExternalLinkage threshold could be tweaked, e.g. module-visible as global.
- // Avoid caching linkage if it may change after enclosing code completion.
- if (hasUnstableLinkage(&D) || D.getLinkageInternal() < ExternalLinkage)
- return SymbolScope::FileScope;
- return SymbolScope::GlobalScope;
-}
-
} // namespace clangd
} // namespace clang
diff --git a/clang-tools-extra/clangd/AST.h b/clang-tools-extra/clangd/AST.h
index 1baf8c585fd2..b603964189e8 100644
--- a/clang-tools-extra/clangd/AST.h
+++ b/clang-tools-extra/clangd/AST.h
@@ -162,15 +162,6 @@ std::string getQualification(ASTContext &Context,
/// the cached value is incorrect. (clang catches this with an assertion).
bool hasUnstableLinkage(const Decl *D);
-/// An approximate measure of where we expect a symbol to be used.
-enum class SymbolScope {
- FunctionScope,
- ClassScope,
- FileScope,
- GlobalScope,
-};
-SymbolScope symbolScope(const NamedDecl &D);
-
} // namespace clangd
} // namespace clang
diff --git a/clang-tools-extra/clangd/CodeComplete.cpp b/clang-tools-extra/clangd/CodeComplete.cpp
index eb0df5fd4dd9..1d1027319dfb 100644
--- a/clang-tools-extra/clangd/CodeComplete.cpp
+++ b/clang-tools-extra/clangd/CodeComplete.cpp
@@ -1726,7 +1726,7 @@ class CodeCompleteFlow {
}
if (Candidate.IdentifierResult) {
Quality.References = Candidate.IdentifierResult->References;
- Relevance.ScopeKind = SymbolScope::FileScope;
+ Relevance.Scope = SymbolRelevanceSignals::FileScope;
Origin |= SymbolOrigin::Identifier;
}
}
diff --git a/clang-tools-extra/clangd/Quality.cpp b/clang-tools-extra/clangd/Quality.cpp
index 28b44bd0d4a1..b49392bc7d04 100644
--- a/clang-tools-extra/clangd/Quality.cpp
+++ b/clang-tools-extra/clangd/Quality.cpp
@@ -262,12 +262,37 @@ llvm::raw_ostream &operator<<(llvm::raw_ostream &OS,
return OS;
}
+static SymbolRelevanceSignals::AccessibleScope
+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();
+ // Class constructor should have the same scope as the class.
+ if (isa<CXXConstructorDecl>(D))
+ DC = DC->getParent();
+ bool InClass = false;
+ for (; !DC->isFileContext(); DC = DC->getParent()) {
+ if (DC->isFunctionOrMethod())
+ return SymbolRelevanceSignals::FunctionScope;
+ InClass = InClass || DC->isRecord();
+ }
+ if (InClass)
+ return SymbolRelevanceSignals::ClassScope;
+ // ExternalLinkage threshold could be tweaked, e.g. module-visible as global.
+ // Avoid caching linkage if it may change after enclosing code completion.
+ if (hasUnstableLinkage(D) || D->getLinkageInternal() < ExternalLinkage)
+ return SymbolRelevanceSignals::FileScope;
+ return SymbolRelevanceSignals::GlobalScope;
+}
+
void SymbolRelevanceSignals::merge(const Symbol &IndexResult) {
SymbolURI = IndexResult.CanonicalDeclaration.FileURI;
- Scope = IndexResult.Scope;
+ SymbolScope = IndexResult.Scope;
IsInstanceMember |= isInstanceMember(IndexResult.SymInfo);
if (!(IndexResult.Flags & Symbol::VisibleOutsideFile)) {
- ScopeKind = SymbolScope::FileScope;
+ Scope = AccessibleScope::FileScope;
}
if (MainFileSignals) {
MainFileRefs =
@@ -325,7 +350,7 @@ void SymbolRelevanceSignals::merge(const CodeCompletionResult &SemaCCResult) {
computeASTSignals(SemaCCResult);
// Declarations are scoped, others (like macros) are assumed global.
if (SemaCCResult.Declaration)
- ScopeKind = std::min(ScopeKind, symbolScope(*SemaCCResult.Declaration));
+ Scope = std::min(Scope, computeScope(SemaCCResult.Declaration));
NeedsFixIts = !SemaCCResult.FixIts.empty();
}
@@ -368,7 +393,7 @@ SymbolRelevanceSignals::calculateDerivedSignals() const {
if (ScopeProximityMatch) {
// For global symbol, the distance is 0.
Derived.ScopeProximityDistance =
- Scope ? ScopeProximityMatch->distance(*Scope) : 0;
+ SymbolScope ? ScopeProximityMatch->distance(*SymbolScope) : 0;
}
return Derived;
}
@@ -404,26 +429,26 @@ float SymbolRelevanceSignals::evaluateHeuristics() const {
if (Query == CodeComplete) {
// The narrower the scope where a symbol is visible, the more likely it is
// to be relevant when it is available.
- switch (ScopeKind) {
- case SymbolScope::GlobalScope:
+ switch (Scope) {
+ case GlobalScope:
break;
- case SymbolScope::FileScope:
+ case FileScope:
Score *= 1.5f;
break;
- case SymbolScope::ClassScope:
+ case ClassScope:
Score *= 2;
break;
- case SymbolScope::FunctionScope:
+ case FunctionScope:
Score *= 4;
break;
}
} else {
// For non-completion queries, the wider the scope where a symbol is
// visible, the more likely it is to be relevant.
- switch (ScopeKind) {
- case SymbolScope::GlobalScope:
+ switch (Scope) {
+ case GlobalScope:
break;
- case SymbolScope::FileScope:
+ case FileScope:
Score *= 0.5f;
break;
default:
@@ -482,11 +507,11 @@ llvm::raw_ostream &operator<<(llvm::raw_ostream &OS,
OS << llvm::formatv("\tInBaseClass: {0}\n", S.InBaseClass);
OS << llvm::formatv("\tContext: {0}\n", getCompletionKindString(S.Context));
OS << llvm::formatv("\tQuery type: {0}\n", static_cast<int>(S.Query));
- OS << llvm::formatv("\tScope: {0}\n", static_cast<int>(S.ScopeKind));
+ OS << llvm::formatv("\tScope: {0}\n", static_cast<int>(S.Scope));
OS << llvm::formatv("\tSymbol URI: {0}\n", S.SymbolURI);
OS << llvm::formatv("\tSymbol scope: {0}\n",
- S.Scope ? *S.Scope : "<None>");
+ S.SymbolScope ? *S.SymbolScope : "<None>");
SymbolRelevanceSignals::DerivedSignals Derived = S.calculateDerivedSignals();
if (S.FileProximityMatch) {
@@ -543,7 +568,7 @@ evaluateDecisionForest(const SymbolQualitySignals &Quality,
E.setSemaFileProximityScore(Relevance.SemaFileProximityScore);
E.setSymbolScopeDistanceCost(Derived.ScopeProximityDistance);
E.setSemaSaysInScope(Relevance.SemaSaysInScope);
- E.setScope(static_cast<uint32_t>(Relevance.ScopeKind));
+ E.setScope(Relevance.Scope);
E.setContextKind(Relevance.Context);
E.setIsInstanceMember(Relevance.IsInstanceMember);
E.setHadContextType(Relevance.HadContextType);
diff --git a/clang-tools-extra/clangd/Quality.h b/clang-tools-extra/clangd/Quality.h
index f4b925e5efbb..08a8981865de 100644
--- a/clang-tools-extra/clangd/Quality.h
+++ b/clang-tools-extra/clangd/Quality.h
@@ -108,11 +108,17 @@ struct SymbolRelevanceSignals {
// Scope proximity is only considered (both index and sema) when this is set.
ScopeDistance *ScopeProximityMatch = nullptr;
- llvm::Optional<llvm::StringRef> Scope;
+ llvm::Optional<llvm::StringRef> SymbolScope;
// A symbol from sema should be accessible from the current scope.
bool SemaSaysInScope = false;
- SymbolScope ScopeKind = SymbolScope::GlobalScope;
+ // An approximate measure of where we expect the symbol to be used.
+ enum AccessibleScope {
+ FunctionScope,
+ ClassScope,
+ FileScope,
+ GlobalScope,
+ } Scope = GlobalScope;
enum QueryType {
CodeComplete,
diff --git a/clang-tools-extra/clangd/quality/CompletionModelCodegen.py b/clang-tools-extra/clangd/quality/CompletionModelCodegen.py
index c93f76d23002..f27e8f6a28a3 100644
--- a/clang-tools-extra/clangd/quality/CompletionModelCodegen.py
+++ b/clang-tools-extra/clangd/quality/CompletionModelCodegen.py
@@ -245,7 +245,7 @@ def gen_cpp_code(forest_json, features_json, filename, cpp_class):
%s
-#define BIT(X) (1u << unsigned(X))
+#define BIT(X) (1 << X)
%s
diff --git a/clang-tools-extra/clangd/quality/model/features.json b/clang-tools-extra/clangd/quality/model/features.json
index 7831506d7f14..2f68848ce807 100644
--- a/clang-tools-extra/clangd/quality/model/features.json
+++ b/clang-tools-extra/clangd/quality/model/features.json
@@ -78,7 +78,7 @@
{
"name": "Scope",
"kind": "ENUM",
- "type": "clang::clangd::SymbolScope",
- "header": "AST.h"
+ "type": "clang::clangd::SymbolRelevanceSignals::AccessibleScope",
+ "header": "Quality.h"
}
]
diff --git a/clang-tools-extra/clangd/unittests/ASTTests.cpp b/clang-tools-extra/clangd/unittests/ASTTests.cpp
index df5af23abc7e..4c52c72d703c 100644
--- a/clang-tools-extra/clangd/unittests/ASTTests.cpp
+++ b/clang-tools-extra/clangd/unittests/ASTTests.cpp
@@ -10,7 +10,6 @@
#include "Annotations.h"
#include "ParsedAST.h"
-#include "SourceCode.h"
#include "TestTU.h"
#include "clang/AST/Decl.h"
#include "clang/AST/DeclBase.h"
@@ -352,76 +351,6 @@ TEST(ClangdAST, PrintType) {
}
}
}
-
-TEST(ASTTest, SymbolScope) {
- const struct {
- const char*Code;
- SymbolScope Expected;
- } Cases[] = {
- {
- "int ^x;",
- SymbolScope::GlobalScope,
- },
- {
- "static int ^x;",
- SymbolScope::FileScope,
- },
- {
- "namespace foo { int ^x; }",
- SymbolScope::GlobalScope,
- },
- {
- "namespace { int ^x; }",
- SymbolScope::FileScope,
- },
- {
- "class X{ void ^Y(); };",
- SymbolScope::ClassScope,
- },
- {
- "class X{ ^X(); };",
- SymbolScope::GlobalScope,
- },
- {
- "class X{ ^~X(); };",
- SymbolScope::ClassScope,
- },
- {
- "void x() { int ^y; }",
- SymbolScope::FunctionScope,
- },
- {
- "void x(int ^y) {}",
- SymbolScope::FunctionScope,
- },
- {
- "void x() { class ^Y{}; }",
- SymbolScope::FunctionScope,
- },
- {
- "void x() { class Y{ ^Y(); }; }",
- SymbolScope::FunctionScope,
- },
- {
- "void x() {auto y = [^z(0)]{};}",
- SymbolScope::FunctionScope,
- },
- };
-
- for (const auto& Case : Cases) {
- SCOPED_TRACE(Case.Code);
- Annotations Test(Case.Code);
- auto AST = TestTU::withCode(Test.code()).build();
-
- SourceLocation Loc = cantFail(
- sourceLocationInMainFile(AST.getSourceManager(), Test.point()));
- const NamedDecl &D = findDecl(
- AST, [&](const NamedDecl &D) { return D.getLocation() == Loc; });
-
- EXPECT_EQ(symbolScope(D), Case.Expected);
- }
-}
-
} // namespace
} // namespace clangd
} // namespace clang
diff --git a/clang-tools-extra/clangd/unittests/QualityTests.cpp b/clang-tools-extra/clangd/unittests/QualityTests.cpp
index bffbab49302d..f5fcb0e8d04e 100644
--- a/clang-tools-extra/clangd/unittests/QualityTests.cpp
+++ b/clang-tools-extra/clangd/unittests/QualityTests.cpp
@@ -122,7 +122,7 @@ TEST(QualityTests, SymbolRelevanceSignalExtraction) {
/*Accessible=*/false));
EXPECT_EQ(Relevance.NameMatch, SymbolRelevanceSignals().NameMatch);
EXPECT_TRUE(Relevance.Forbidden);
- EXPECT_EQ(Relevance.ScopeKind, SymbolScope::GlobalScope);
+ EXPECT_EQ(Relevance.Scope, SymbolRelevanceSignals::GlobalScope);
Relevance = {};
Relevance.merge(CodeCompletionResult(&findDecl(AST, "main"), 42));
@@ -160,17 +160,17 @@ TEST(QualityTests, SymbolRelevanceSignalExtraction) {
Relevance = {};
Relevance.merge(CodeCompletionResult(&findUnqualifiedDecl(AST, "X"), 42));
- EXPECT_EQ(Relevance.ScopeKind, SymbolScope::FileScope);
+ EXPECT_EQ(Relevance.Scope, SymbolRelevanceSignals::FileScope);
Relevance = {};
Relevance.merge(CodeCompletionResult(&findUnqualifiedDecl(AST, "y"), 42));
- EXPECT_EQ(Relevance.ScopeKind, SymbolScope::ClassScope);
+ EXPECT_EQ(Relevance.Scope, SymbolRelevanceSignals::ClassScope);
Relevance = {};
Relevance.merge(CodeCompletionResult(&findUnqualifiedDecl(AST, "z"), 42));
- EXPECT_EQ(Relevance.ScopeKind, SymbolScope::FunctionScope);
+ 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.ScopeKind, SymbolScope::GlobalScope);
+ EXPECT_EQ(Relevance.Scope, SymbolRelevanceSignals::GlobalScope);
Relevance = {};
EXPECT_FALSE(Relevance.InBaseClass);
@@ -188,7 +188,7 @@ TEST(QualityTests, SymbolRelevanceSignalExtraction) {
Matched = true;
Relevance = {};
Relevance.merge(S);
- EXPECT_EQ(Relevance.ScopeKind, SymbolScope::FileScope);
+ EXPECT_EQ(Relevance.Scope, SymbolRelevanceSignals::FileScope);
});
EXPECT_TRUE(Matched);
}
@@ -263,7 +263,7 @@ TEST(QualityTests, SymbolRelevanceSignalsSanity) {
SymbolRelevanceSignals WithIndexScopeProximity;
WithIndexScopeProximity.ScopeProximityMatch = &ScopeProximity;
- WithIndexScopeProximity.Scope = "x::";
+ WithIndexScopeProximity.SymbolScope = "x::";
EXPECT_GT(WithSemaScopeProximity.evaluateHeuristics(),
Default.evaluateHeuristics());
@@ -282,7 +282,7 @@ TEST(QualityTests, SymbolRelevanceSignalsSanity) {
EXPECT_GT(IndexDistant.evaluateHeuristics(), Default.evaluateHeuristics());
SymbolRelevanceSignals Scoped;
- Scoped.ScopeKind = SymbolScope::FileScope;
+ Scoped.Scope = SymbolRelevanceSignals::FileScope;
EXPECT_LT(Scoped.evaluateHeuristics(), Default.evaluateHeuristics());
Scoped.Query = SymbolRelevanceSignals::CodeComplete;
EXPECT_GT(Scoped.evaluateHeuristics(), Default.evaluateHeuristics());
@@ -317,26 +317,26 @@ TEST(QualityTests, ScopeProximity) {
ScopeDistance ScopeProximity({"x::y::z::", "x::", "llvm::", ""});
Relevance.ScopeProximityMatch = &ScopeProximity;
- Relevance.Scope = "other::";
+ Relevance.SymbolScope = "other::";
float NotMatched = Relevance.evaluateHeuristics();
- Relevance.Scope = "";
+ Relevance.SymbolScope = "";
float Global = Relevance.evaluateHeuristics();
EXPECT_GT(Global, NotMatched);
- Relevance.Scope = "llvm::";
+ Relevance.SymbolScope = "llvm::";
float NonParent = Relevance.evaluateHeuristics();
EXPECT_GT(NonParent, Global);
- Relevance.Scope = "x::";
+ Relevance.SymbolScope = "x::";
float GrandParent = Relevance.evaluateHeuristics();
EXPECT_GT(GrandParent, Global);
- Relevance.Scope = "x::y::";
+ Relevance.SymbolScope = "x::y::";
float Parent = Relevance.evaluateHeuristics();
EXPECT_GT(Parent, GrandParent);
- Relevance.Scope = "x::y::z::";
+ Relevance.SymbolScope = "x::y::z::";
float Enclosing = Relevance.evaluateHeuristics();
EXPECT_GT(Enclosing, Parent);
}
@@ -375,8 +375,8 @@ TEST(QualityTests, NoBoostForClassConstructor) {
SymbolRelevanceSignals Ctor;
Ctor.merge(CodeCompletionResult(CtorDecl, /*Priority=*/0));
- EXPECT_EQ(Cls.ScopeKind, SymbolScope::GlobalScope);
- EXPECT_EQ(Ctor.ScopeKind, SymbolScope::GlobalScope);
+ EXPECT_EQ(Cls.Scope, SymbolRelevanceSignals::GlobalScope);
+ EXPECT_EQ(Ctor.Scope, SymbolRelevanceSignals::GlobalScope);
}
TEST(QualityTests, IsInstanceMember) {
More information about the cfe-commits
mailing list