[PATCH] D58448: [clangd] Improve global code completion when scope specifier is unresolved.
This revision was automatically updated to reflect the committed changes.
Closed by commit rL354963: [clangd] Improve global code completion when scope
specifier is unresolved. (authored by ioeric, committed by ).
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.
Repository:
rL LLVM
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D58448/new/
https://reviews.llvm.org/D58448
Files:
clang-tools-extra/trunk/clangd/CodeComplete.cpp
clang-tools-extra/trunk/unittests/clangd/CodeCompleteTests.cpp
Index: clang-tools-extra/trunk/unittests/clangd/CodeCompleteTests.cpp
===
--- clang-tools-extra/trunk/unittests/clangd/CodeCompleteTests.cpp
+++ clang-tools-extra/trunk/unittests/clangd/CodeCompleteTests.cpp
@@ -1095,8 +1095,10 @@
} // namespace ns
)cpp");
- EXPECT_THAT(Requests, ElementsAre(Field(&FuzzyFindRequest::Scopes,
- UnorderedElementsAre("bar::";
+ EXPECT_THAT(Requests,
+ ElementsAre(Field(
+ &FuzzyFindRequest::Scopes,
+ UnorderedElementsAre("a::bar::", "ns::bar::", "bar::";
}
TEST(CompletionTest, UnresolvedNestedQualifierIdQuery) {
@@ -2335,6 +2337,35 @@
Kind(CompletionItemKind::Reference;
}
+TEST(CompletionTest, ScopeIsUnresolved) {
+ clangd::CodeCompleteOptions Opts = {};
+ Opts.AllScopes = true;
+
+ auto Results = completions(R"cpp(
+namespace a {
+void f() { b::X^ }
+}
+ )cpp",
+ {cls("a::b::XYZ")}, Opts);
+ EXPECT_THAT(Results.Completions,
+ UnorderedElementsAre(AllOf(Qualifier(""), Named("XYZ";
+}
+
+TEST(CompletionTest, NestedScopeIsUnresolved) {
+ clangd::CodeCompleteOptions Opts = {};
+ Opts.AllScopes = true;
+
+ auto Results = completions(R"cpp(
+namespace a {
+namespace b {}
+void f() { b::c::X^ }
+}
+ )cpp",
+ {cls("a::b::c::XYZ")}, Opts);
+ EXPECT_THAT(Results.Completions,
+ UnorderedElementsAre(AllOf(Qualifier(""), Named("XYZ";
+}
+
} // namespace
} // namespace clangd
} // namespace clang
Index: clang-tools-extra/trunk/clangd/CodeComplete.cpp
===
--- clang-tools-extra/trunk/clangd/CodeComplete.cpp
+++ clang-tools-extra/trunk/clangd/CodeComplete.cpp
@@ -48,6 +48,7 @@
#include "llvm/ADT/None.h"
#include "llvm/ADT/Optional.h"
#include "llvm/ADT/SmallVector.h"
+#include "llvm/ADT/StringRef.h"
#include "llvm/Support/Error.h"
#include "llvm/Support/Format.h"
#include "llvm/Support/FormatVariadic.h"
@@ -526,7 +527,7 @@
std::set Results;
for (llvm::StringRef AS : AccessibleScopes)
Results.insert(
- ((UnresolvedQualifier ? *UnresolvedQualifier : "") + AS).str());
+ (AS + (UnresolvedQualifier ? *UnresolvedQualifier : "")).str());
return {Results.begin(), Results.end()};
}
};
@@ -570,16 +571,15 @@
}
// Unresolved qualifier.
- // FIXME: When Sema can resolve part of a scope chain (e.g.
- // "known::unknown::id"), we should expand the known part ("known::") rather
- // than treating the whole thing as unknown.
- SpecifiedScope Info;
- Info.AccessibleScopes.push_back(""); // global namespace
+ SpecifiedScope Info = GetAllAccessibleScopes(CCContext);
+ Info.AccessibleScopes.push_back(""); // Make sure global scope is included.
- Info.UnresolvedQualifier =
+ llvm::StringRef SpelledSpecifier =
Lexer::getSourceText(CharSourceRange::getCharRange((*SS)->getRange()),
- CCSema.SourceMgr, clang::LangOptions())
- .ltrim("::");
+ CCSema.SourceMgr, clang::LangOptions());
+ if (SpelledSpecifier.consume_front("::"))
+Info.AccessibleScopes = {""};
+ Info.UnresolvedQualifier = SpelledSpecifier;
// Sema excludes the trailing "::".
if (!Info.UnresolvedQualifier->empty())
*Info.UnresolvedQualifier += "::";
Index: clang-tools-extra/trunk/unittests/clangd/CodeCompleteTests.cpp
===
--- clang-tools-extra/trunk/unittests/clangd/CodeCompleteTests.cpp
+++ clang-tools-extra/trunk/unittests/clangd/CodeCompleteTests.cpp
@@ -1095,8 +1095,10 @@
} // namespace ns
)cpp");
- EXPECT_THAT(Requests, ElementsAre(Field(&FuzzyFindRequest::Scopes,
- UnorderedElementsAre("bar::";
+ EXPECT_THAT(Requests,
+ ElementsAre(Field(
+ &FuzzyFindRequest::Scopes,
+ UnorderedElementsAre("a::bar::", "ns::bar::", "bar::";
}
TEST(CompletionTest, UnresolvedNestedQualifierIdQuery) {
@@ -2335,6 +2337,35 @@
Kind(CompletionItemKind::Reference;
}
+TEST(CompletionTest, ScopeIsUnresolved) {
+ clangd::CodeCompleteOptions Opts = {};
+ Opts.AllScopes = true;
+
+ au
[PATCH] D58448: [clangd] Improve global code completion when scope specifier is unresolved.
ioeric updated this revision to Diff 188512.
ioeric added a comment.
- Rebase and minor cleanup
Repository:
rCTE Clang Tools Extra
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D58448/new/
https://reviews.llvm.org/D58448
Files:
clangd/CodeComplete.cpp
unittests/clangd/CodeCompleteTests.cpp
Index: unittests/clangd/CodeCompleteTests.cpp
===
--- unittests/clangd/CodeCompleteTests.cpp
+++ unittests/clangd/CodeCompleteTests.cpp
@@ -1095,8 +1095,10 @@
} // namespace ns
)cpp");
- EXPECT_THAT(Requests, ElementsAre(Field(&FuzzyFindRequest::Scopes,
- UnorderedElementsAre("bar::";
+ EXPECT_THAT(Requests,
+ ElementsAre(Field(
+ &FuzzyFindRequest::Scopes,
+ UnorderedElementsAre("a::bar::", "ns::bar::", "bar::";
}
TEST(CompletionTest, UnresolvedNestedQualifierIdQuery) {
@@ -2335,6 +2337,35 @@
Kind(CompletionItemKind::Reference;
}
+TEST(CompletionTest, ScopeIsUnresolved) {
+ clangd::CodeCompleteOptions Opts = {};
+ Opts.AllScopes = true;
+
+ auto Results = completions(R"cpp(
+namespace a {
+void f() { b::X^ }
+}
+ )cpp",
+ {cls("a::b::XYZ")}, Opts);
+ EXPECT_THAT(Results.Completions,
+ UnorderedElementsAre(AllOf(Qualifier(""), Named("XYZ";
+}
+
+TEST(CompletionTest, NestedScopeIsUnresolved) {
+ clangd::CodeCompleteOptions Opts = {};
+ Opts.AllScopes = true;
+
+ auto Results = completions(R"cpp(
+namespace a {
+namespace b {}
+void f() { b::c::X^ }
+}
+ )cpp",
+ {cls("a::b::c::XYZ")}, Opts);
+ EXPECT_THAT(Results.Completions,
+ UnorderedElementsAre(AllOf(Qualifier(""), Named("XYZ";
+}
+
} // namespace
} // namespace clangd
} // namespace clang
Index: clangd/CodeComplete.cpp
===
--- clangd/CodeComplete.cpp
+++ clangd/CodeComplete.cpp
@@ -48,6 +48,7 @@
#include "llvm/ADT/None.h"
#include "llvm/ADT/Optional.h"
#include "llvm/ADT/SmallVector.h"
+#include "llvm/ADT/StringRef.h"
#include "llvm/Support/Error.h"
#include "llvm/Support/Format.h"
#include "llvm/Support/FormatVariadic.h"
@@ -526,7 +527,7 @@
std::set Results;
for (llvm::StringRef AS : AccessibleScopes)
Results.insert(
- ((UnresolvedQualifier ? *UnresolvedQualifier : "") + AS).str());
+ (AS + (UnresolvedQualifier ? *UnresolvedQualifier : "")).str());
return {Results.begin(), Results.end()};
}
};
@@ -570,16 +571,15 @@
}
// Unresolved qualifier.
- // FIXME: When Sema can resolve part of a scope chain (e.g.
- // "known::unknown::id"), we should expand the known part ("known::") rather
- // than treating the whole thing as unknown.
- SpecifiedScope Info;
- Info.AccessibleScopes.push_back(""); // global namespace
+ SpecifiedScope Info = GetAllAccessibleScopes(CCContext);
+ Info.AccessibleScopes.push_back(""); // Make sure global scope is included.
- Info.UnresolvedQualifier =
+ llvm::StringRef SpelledSpecifier =
Lexer::getSourceText(CharSourceRange::getCharRange((*SS)->getRange()),
- CCSema.SourceMgr, clang::LangOptions())
- .ltrim("::");
+ CCSema.SourceMgr, clang::LangOptions());
+ if (SpelledSpecifier.consume_front("::"))
+Info.AccessibleScopes = {""};
+ Info.UnresolvedQualifier = SpelledSpecifier;
// Sema excludes the trailing "::".
if (!Info.UnresolvedQualifier->empty())
*Info.UnresolvedQualifier += "::";
Index: unittests/clangd/CodeCompleteTests.cpp
===
--- unittests/clangd/CodeCompleteTests.cpp
+++ unittests/clangd/CodeCompleteTests.cpp
@@ -1095,8 +1095,10 @@
} // namespace ns
)cpp");
- EXPECT_THAT(Requests, ElementsAre(Field(&FuzzyFindRequest::Scopes,
- UnorderedElementsAre("bar::";
+ EXPECT_THAT(Requests,
+ ElementsAre(Field(
+ &FuzzyFindRequest::Scopes,
+ UnorderedElementsAre("a::bar::", "ns::bar::", "bar::";
}
TEST(CompletionTest, UnresolvedNestedQualifierIdQuery) {
@@ -2335,6 +2337,35 @@
Kind(CompletionItemKind::Reference;
}
+TEST(CompletionTest, ScopeIsUnresolved) {
+ clangd::CodeCompleteOptions Opts = {};
+ Opts.AllScopes = true;
+
+ auto Results = completions(R"cpp(
+namespace a {
+void f() { b::X^ }
+}
+ )cpp",
+ {cls("a::b::XYZ")}, Opts);
+ EXPECT_THAT(Results.Completions,
+ UnorderedElementsAre(AllOf(Qualifier(""), Named("XYZ";
+}
+
+TEST(CompletionTest, NestedScopeIsUnresolved) {
+ clangd::CodeCompleteOptions Opts = {};
+ Opts.AllScopes = true;
+
+ auto Results = completions(R"cpp(
+namespace a
Re: [PATCH] D58448: [clangd] Improve global code completion when scope specifier is unresolved.
Slightly offtopic. It would be nice to add a way to collect quality during actual completion sessions, rather than simulated ones. I'm thinking about clients sending a message to the server mentioning the completion item user ended up selecting in a completion list (would require an LSP extension). I believe this should be enough information to collect metrics for the use of code completion in the wild. On Tue, Feb 26, 2019 at 11:04 AM Eric Liu wrote: > Unfortunately, the evaluation tool I use only works on compilable code, so > it doesn't capture the unsolved specifier case in this patch. I didn't try > to collect the metrics because I think this is more of a bug fix than > quality improvement. > > On Tue, Feb 26, 2019, 10:25 Kadir Cetinkaya via Phabricator < > revi...@reviews.llvm.org> wrote: > >> kadircet added a comment. >> >> LG >> >> Do we have any metrics regarding change in completion quality? >> >> >> Repository: >> rCTE Clang Tools Extra >> >> CHANGES SINCE LAST ACTION >> https://reviews.llvm.org/D58448/new/ >> >> https://reviews.llvm.org/D58448 >> >> >> >> -- Regards, Ilya Biryukov ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D58448: [clangd] Improve global code completion when scope specifier is unresolved.
Unfortunately, the evaluation tool I use only works on compilable code, so it doesn't capture the unsolved specifier case in this patch. I didn't try to collect the metrics because I think this is more of a bug fix than quality improvement. On Tue, Feb 26, 2019, 10:25 Kadir Cetinkaya via Phabricator < revi...@reviews.llvm.org> wrote: > kadircet added a comment. > > LG > > Do we have any metrics regarding change in completion quality? > > > Repository: > rCTE Clang Tools Extra > > CHANGES SINCE LAST ACTION > https://reviews.llvm.org/D58448/new/ > > https://reviews.llvm.org/D58448 > > > > ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D58448: [clangd] Improve global code completion when scope specifier is unresolved.
kadircet added a comment. LG Do we have any metrics regarding change in completion quality? Repository: rCTE Clang Tools Extra CHANGES SINCE LAST ACTION https://reviews.llvm.org/D58448/new/ https://reviews.llvm.org/D58448 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D58448: [clangd] Improve global code completion when scope specifier is unresolved.
ioeric created this revision.
ioeric added reviewers: ilya-biryukov, sammccall.
Herald added subscribers: cfe-commits, jdoerfert, kadircet, arphaman, jkorous,
MaskRay.
Herald added a project: clang.
Suppose `clangd::` is unresolved in the following example. Currently,
we simply use "clangd::" as the query scope. We can do better by combining with
accessible scopes in the context. The query scopes can be `{clangd::,
clang::clangd::}`.
namespace clang { clangd::^ }
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D58448
Files:
clangd/CodeComplete.cpp
unittests/clangd/CodeCompleteTests.cpp
Index: unittests/clangd/CodeCompleteTests.cpp
===
--- unittests/clangd/CodeCompleteTests.cpp
+++ unittests/clangd/CodeCompleteTests.cpp
@@ -1094,8 +1094,10 @@
} // namespace ns
)cpp");
- EXPECT_THAT(Requests, ElementsAre(Field(&FuzzyFindRequest::Scopes,
- UnorderedElementsAre("bar::";
+ EXPECT_THAT(Requests,
+ ElementsAre(Field(
+ &FuzzyFindRequest::Scopes,
+ UnorderedElementsAre("a::bar::", "ns::bar::", "bar::";
}
TEST(CompletionTest, UnresolvedNestedQualifierIdQuery) {
@@ -2314,6 +2316,35 @@
EXPECT_THAT(R.Completions, ElementsAre(Named("loopVar")));
}
+TEST(CompletionTest, ScopeIsUnresolved) {
+ clangd::CodeCompleteOptions Opts = {};
+ Opts.AllScopes = true;
+
+ auto Results = completions(R"cpp(
+namespace a {
+void f() { b::X^ }
+}
+ )cpp",
+ {cls("a::b::XYZ")}, Opts);
+ EXPECT_THAT(Results.Completions,
+ UnorderedElementsAre(AllOf(Qualifier(""), Named("XYZ";
+}
+
+TEST(CompletionTest, NestedScopeIsUnresolved) {
+ clangd::CodeCompleteOptions Opts = {};
+ Opts.AllScopes = true;
+
+ auto Results = completions(R"cpp(
+namespace a {
+namespace b {}
+void f() { b::c::X^ }
+}
+ )cpp",
+ {cls("a::b::c::XYZ")}, Opts);
+ EXPECT_THAT(Results.Completions,
+ UnorderedElementsAre(AllOf(Qualifier(""), Named("XYZ";
+}
+
} // namespace
} // namespace clangd
} // namespace clang
Index: clangd/CodeComplete.cpp
===
--- clangd/CodeComplete.cpp
+++ clangd/CodeComplete.cpp
@@ -48,6 +48,7 @@
#include "llvm/ADT/None.h"
#include "llvm/ADT/Optional.h"
#include "llvm/ADT/SmallVector.h"
+#include "llvm/ADT/StringRef.h"
#include "llvm/Support/Error.h"
#include "llvm/Support/Format.h"
#include "llvm/Support/FormatVariadic.h"
@@ -526,7 +527,7 @@
std::set Results;
for (llvm::StringRef AS : AccessibleScopes)
Results.insert(
- ((UnresolvedQualifier ? *UnresolvedQualifier : "") + AS).str());
+ (AS + (UnresolvedQualifier ? *UnresolvedQualifier : "")).str());
return {Results.begin(), Results.end()};
}
};
@@ -570,16 +571,16 @@
}
// Unresolved qualifier.
- // FIXME: When Sema can resolve part of a scope chain (e.g.
- // "known::unknown::id"), we should expand the known part ("known::") rather
- // than treating the whole thing as unknown.
- SpecifiedScope Info;
- Info.AccessibleScopes.push_back(""); // global namespace
+ SpecifiedScope Info = GetAllAccessibleScopes(CCContext);
+ if (Info.AccessibleScopes.empty())
+Info.AccessibleScopes.push_back(""); // Fallback to global namespace.
- Info.UnresolvedQualifier =
+ llvm::StringRef SpelledSpecifier =
Lexer::getSourceText(CharSourceRange::getCharRange((*SS)->getRange()),
- CCSema.SourceMgr, clang::LangOptions())
- .ltrim("::");
+ CCSema.SourceMgr, clang::LangOptions());
+ if (SpelledSpecifier.consume_front("::"))
+Info.AccessibleScopes = {""};
+ Info.UnresolvedQualifier = SpelledSpecifier;
// Sema excludes the trailing "::".
if (!Info.UnresolvedQualifier->empty())
*Info.UnresolvedQualifier += "::";
Index: unittests/clangd/CodeCompleteTests.cpp
===
--- unittests/clangd/CodeCompleteTests.cpp
+++ unittests/clangd/CodeCompleteTests.cpp
@@ -1094,8 +1094,10 @@
} // namespace ns
)cpp");
- EXPECT_THAT(Requests, ElementsAre(Field(&FuzzyFindRequest::Scopes,
- UnorderedElementsAre("bar::";
+ EXPECT_THAT(Requests,
+ ElementsAre(Field(
+ &FuzzyFindRequest::Scopes,
+ UnorderedElementsAre("a::bar::", "ns::bar::", "bar::";
}
TEST(CompletionTest, UnresolvedNestedQualifierIdQuery) {
@@ -2314,6 +2316,35 @@
EXPECT_THAT(R.Completions, ElementsAre(Named("loopVar")));
}
+TEST(CompletionTest, ScopeIsUnresolved) {
+ clangd::CodeCompleteOptions Opts = {};
+ Opts.AllScopes = true;
+
+ auto Results = completions(R"cpp(
+namespace a {
+void f() { b::X^ }
+}
+ )cpp",
+
