[PATCH] D39836: [clangd] Drop impossible completions (unavailable or inaccessible)
This revision was automatically updated to reflect the committed changes.
Closed by commit rL318925: [clangd] Drop impossible completions (unavailable or
inaccessible) (authored by sammccall).
Repository:
rL LLVM
https://reviews.llvm.org/D39836
Files:
clang-tools-extra/trunk/clangd/ClangdLSPServer.cpp
clang-tools-extra/trunk/clangd/ClangdLSPServer.h
clang-tools-extra/trunk/clangd/ClangdServer.cpp
clang-tools-extra/trunk/clangd/ClangdServer.h
clang-tools-extra/trunk/clangd/ClangdUnit.cpp
clang-tools-extra/trunk/clangd/ClangdUnit.h
clang-tools-extra/trunk/clangd/tool/ClangdMain.cpp
clang-tools-extra/trunk/test/clangd/completion-items-kinds.test
clang-tools-extra/trunk/test/clangd/completion-priorities.test
clang-tools-extra/trunk/test/clangd/completion-qualifiers.test
clang-tools-extra/trunk/unittests/clangd/ClangdTests.cpp
Index: clang-tools-extra/trunk/unittests/clangd/ClangdTests.cpp
===
--- clang-tools-extra/trunk/unittests/clangd/ClangdTests.cpp
+++ clang-tools-extra/trunk/unittests/clangd/ClangdTests.cpp
@@ -788,6 +788,8 @@
int method();
int field;
+private:
+ int private_field;
};
int test() {
@@ -828,7 +830,10 @@
// Class members. The only items that must be present in after-dor
// completion.
EXPECT_TRUE(ContainsItem(Results, MethodItemText));
+ EXPECT_TRUE(ContainsItem(Results, MethodItemText));
EXPECT_TRUE(ContainsItem(Results, "field"));
+ EXPECT_EQ(Opts.IncludeIneligibleResults,
+ContainsItem(Results, "private_field"));
// Global items.
EXPECT_FALSE(ContainsItem(Results, "global_var"));
EXPECT_FALSE(ContainsItem(Results, GlobalFuncItemText));
@@ -889,18 +894,26 @@
}
};
- for (bool IncludeMacros : {true, false})
-for (bool IncludeGlobals : {true, false})
- for (bool IncludeBriefComments : {true, false})
-for (bool EnableSnippets : {true, false})
+ clangd::CodeCompleteOptions CCOpts;
+ for (bool IncludeMacros : {true, false}){
+CCOpts.IncludeMacros = IncludeMacros;
+for (bool IncludeGlobals : {true, false}){
+ CCOpts.IncludeGlobals = IncludeGlobals;
+ for (bool IncludeBriefComments : {true, false}){
+CCOpts.IncludeBriefComments = IncludeBriefComments;
+for (bool EnableSnippets : {true, false}){
+ CCOpts.EnableSnippets = EnableSnippets;
for (bool IncludeCodePatterns : {true, false}) {
-TestWithOpts(clangd::CodeCompleteOptions(
-/*EnableSnippets=*/EnableSnippets,
-/*IncludeCodePatterns=*/IncludeCodePatterns,
-/*IncludeMacros=*/IncludeMacros,
-/*IncludeGlobals=*/IncludeGlobals,
-/*IncludeBriefComments=*/IncludeBriefComments));
+CCOpts.IncludeCodePatterns = IncludeCodePatterns;
+for (bool IncludeIneligibleResults : {true, false}) {
+ CCOpts.IncludeIneligibleResults = IncludeIneligibleResults;
+ TestWithOpts(CCOpts);
+}
}
+}
+ }
+}
+ }
}
class ClangdThreadingTest : public ClangdVFSTest {};
Index: clang-tools-extra/trunk/clangd/ClangdServer.h
===
--- clang-tools-extra/trunk/clangd/ClangdServer.h
+++ clang-tools-extra/trunk/clangd/ClangdServer.h
@@ -182,9 +182,6 @@
/// AsyncThreadsCount worker threads. However, if \p AsyncThreadsCount is 0,
/// all requests will be processed on the calling thread.
///
- /// When \p SnippetCompletions is true, completion items will be presented
- /// with embedded snippets. Otherwise, plaintext items will be presented.
- ///
/// ClangdServer uses \p FSProvider to get an instance of vfs::FileSystem for
/// each parsing request. Results of code completion and diagnostics also
/// include a tag, that \p FSProvider returns along with the vfs::FileSystem.
@@ -213,7 +210,7 @@
DiagnosticsConsumer ,
FileSystemProvider , unsigned AsyncThreadsCount,
bool StorePreamblesInMemory,
- clangd::CodeCompleteOptions CodeCompleteOpts,
+ const clangd::CodeCompleteOptions ,
clangd::Logger ,
llvm::Optional ResourceDir = llvm::None);
Index: clang-tools-extra/trunk/clangd/ClangdServer.cpp
===
--- clang-tools-extra/trunk/clangd/ClangdServer.cpp
+++ clang-tools-extra/trunk/clangd/ClangdServer.cpp
@@ -169,11 +169,14 @@
Worker.join();
}
-ClangdServer::ClangdServer(
-GlobalCompilationDatabase , DiagnosticsConsumer ,
-FileSystemProvider , unsigned AsyncThreadsCount,
-bool StorePreamblesInMemory, clangd::CodeCompleteOptions CodeCompleteOpts,
-clangd::Logger , llvm::Optional ResourceDir)
+ClangdServer::ClangdServer(GlobalCompilationDatabase ,
+
[PATCH] D39836: [clangd] Drop impossible completions (unavailable or inaccessible)
sammccall updated this revision to Diff 124066.
sammccall added a comment.
Address review comments
https://reviews.llvm.org/D39836
Files:
clangd/ClangdLSPServer.cpp
clangd/ClangdLSPServer.h
clangd/ClangdServer.cpp
clangd/ClangdServer.h
clangd/ClangdUnit.cpp
clangd/ClangdUnit.h
clangd/tool/ClangdMain.cpp
test/clangd/completion-items-kinds.test
test/clangd/completion-priorities.test
test/clangd/completion-qualifiers.test
unittests/clangd/ClangdTests.cpp
Index: unittests/clangd/ClangdTests.cpp
===
--- unittests/clangd/ClangdTests.cpp
+++ unittests/clangd/ClangdTests.cpp
@@ -788,6 +788,8 @@
int method();
int field;
+private:
+ int private_field;
};
int test() {
@@ -828,7 +830,10 @@
// Class members. The only items that must be present in after-dor
// completion.
EXPECT_TRUE(ContainsItem(Results, MethodItemText));
+ EXPECT_TRUE(ContainsItem(Results, MethodItemText));
EXPECT_TRUE(ContainsItem(Results, "field"));
+ EXPECT_EQ(Opts.IncludeIneligibleResults,
+ContainsItem(Results, "private_field"));
// Global items.
EXPECT_FALSE(ContainsItem(Results, "global_var"));
EXPECT_FALSE(ContainsItem(Results, GlobalFuncItemText));
@@ -889,18 +894,26 @@
}
};
- for (bool IncludeMacros : {true, false})
-for (bool IncludeGlobals : {true, false})
- for (bool IncludeBriefComments : {true, false})
-for (bool EnableSnippets : {true, false})
+ clangd::CodeCompleteOptions CCOpts;
+ for (bool IncludeMacros : {true, false}){
+CCOpts.IncludeMacros = IncludeMacros;
+for (bool IncludeGlobals : {true, false}){
+ CCOpts.IncludeGlobals = IncludeGlobals;
+ for (bool IncludeBriefComments : {true, false}){
+CCOpts.IncludeBriefComments = IncludeBriefComments;
+for (bool EnableSnippets : {true, false}){
+ CCOpts.EnableSnippets = EnableSnippets;
for (bool IncludeCodePatterns : {true, false}) {
-TestWithOpts(clangd::CodeCompleteOptions(
-/*EnableSnippets=*/EnableSnippets,
-/*IncludeCodePatterns=*/IncludeCodePatterns,
-/*IncludeMacros=*/IncludeMacros,
-/*IncludeGlobals=*/IncludeGlobals,
-/*IncludeBriefComments=*/IncludeBriefComments));
+CCOpts.IncludeCodePatterns = IncludeCodePatterns;
+for (bool IncludeIneligibleResults : {true, false}) {
+ CCOpts.IncludeIneligibleResults = IncludeIneligibleResults;
+ TestWithOpts(CCOpts);
+}
}
+}
+ }
+}
+ }
}
class ClangdThreadingTest : public ClangdVFSTest {};
Index: test/clangd/completion-qualifiers.test
===
--- test/clangd/completion-qualifiers.test
+++ test/clangd/completion-qualifiers.test
@@ -13,7 +13,7 @@
# CHECK-NEXT: "result": {
# CHECK-NEXT:"isIncomplete": false,
# CHECK-NEXT:"items": [
-# Eligible const functions are at the top of the list.
+# Eligible functions are at the top of the list.
# CHECK-NEXT: {
# CHECK-NEXT:"detail": "int",
# CHECK-NEXT:"filterText": "bar",
@@ -32,18 +32,9 @@
# CHECK-NEXT:"label": "Foo::foo() const",
# CHECK-NEXT:"sortText": "37foo"
# CHECK-NEXT: },
-# Ineligible non-const function is at the bottom of the list.
-# CHECK-NEXT: {
-# CHECK:"detail": "int",
-# CHECK:"filterText": "foo",
-# CHECK-NEXT:"insertText": "foo",
-# CHECK-NEXT:"insertTextFormat": 1,
-# CHECK-NEXT:"kind": 2,
-# CHECK-NEXT:"label": "foo() const",
-# CHECK-NEXT:"sortText": "200035foo"
-# CHECK-NEXT: }
-# CHECK-NEXT:]
-# CHECK-NEXT: }
+# Ineligible private functions are not present.
+# CHECK-NOT:"label": "foo() const",
+# CHECK:]
Content-Length: 44
{"jsonrpc":"2.0","id":4,"method":"shutdown"}
Index: test/clangd/completion-priorities.test
===
--- test/clangd/completion-priorities.test
+++ test/clangd/completion-priorities.test
@@ -61,28 +61,10 @@
# CHECK-NEXT:"kind": 2,
# CHECK-NEXT:"label": "pub()",
# CHECK-NEXT:"sortText": "34pub"
-# CHECK-NEXT: },
-# priv() and prot() are at the end of the list
-# CHECK-NEXT: {
-# CHECK:"detail": "void",
-# CHECK:"filterText": "priv",
-# CHECK-NEXT:"insertText": "priv",
-# CHECK-NEXT:"insertTextFormat": 1,
-# CHECK-NEXT:"kind": 2,
-# CHECK-NEXT:"label": "priv()",
-# CHECK-NEXT:"sortText": "200034priv"
-# CHECK-NEXT: },
-# CHECK-NEXT: {
-# CHECK-NEXT:"detail": "void",
-# CHECK-NEXT:"filterText": "prot",
-# CHECK-NEXT:"insertText": "prot",
-# CHECK-NEXT:"insertTextFormat": 1,
-# CHECK-NEXT:
[PATCH] D39836: [clangd] Drop impossible completions (unavailable or inaccessible)
sammccall added inline comments. Comment at: clangd/tool/ClangdMain.cpp:169 + clangd::CodeCompleteOptions CCOpts; + CCOpts.EnableSnippets = EnableSnippets; + CCOpts.IncludeIneligibleResults = IncludeIneligibleResults; ilya-biryukov wrote: > We should also set `IncludeCodePatterns = EnableSnippets` here. May be worth > adding a test that they are in the completion results. Good catch! Unfortunately our previous attempt this failed because static_cast is not a code pattern (probably it should be). - Added a test to completion-items-kinds. - Added documentation to the flag to indicate it does this - Changed the CodePatterns default to true (equivalent to setting it here, and keeps the defaults in one place) - set flag defaults based on CompleteOption defaults so they're in one place https://reviews.llvm.org/D39836 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D39836: [clangd] Drop impossible completions (unavailable or inaccessible)
ilya-biryukov added inline comments. Comment at: clangd/tool/ClangdMain.cpp:169 + clangd::CodeCompleteOptions CCOpts; + CCOpts.EnableSnippets = EnableSnippets; + CCOpts.IncludeIneligibleResults = IncludeIneligibleResults; We should also set `IncludeCodePatterns = EnableSnippets` here. May be worth adding a test that they are in the completion results. https://reviews.llvm.org/D39836 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D39836: [clangd] Drop impossible completions (unavailable or inaccessible)
sammccall requested review of this revision. sammccall added a comment. PTAL - this is now optional. The results will be hidden by default, but can be turned on with a flag. https://reviews.llvm.org/D39836 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D39836: [clangd] Drop impossible completions (unavailable or inaccessible)
sammccall updated this revision to Diff 123819.
sammccall added a comment.
(trying to resync the diff to head)
https://reviews.llvm.org/D39836
Files:
clangd/ClangdLSPServer.cpp
clangd/ClangdLSPServer.h
clangd/ClangdServer.cpp
clangd/ClangdServer.h
clangd/ClangdUnit.cpp
clangd/ClangdUnit.h
clangd/tool/ClangdMain.cpp
test/clangd/completion-priorities.test
test/clangd/completion-qualifiers.test
unittests/clangd/ClangdTests.cpp
Index: unittests/clangd/ClangdTests.cpp
===
--- unittests/clangd/ClangdTests.cpp
+++ unittests/clangd/ClangdTests.cpp
@@ -788,6 +788,8 @@
int method();
int field;
+private:
+ int private_field;
};
int test() {
@@ -828,7 +830,10 @@
// Class members. The only items that must be present in after-dor
// completion.
EXPECT_TRUE(ContainsItem(Results, MethodItemText));
+ EXPECT_TRUE(ContainsItem(Results, MethodItemText));
EXPECT_TRUE(ContainsItem(Results, "field"));
+ EXPECT_EQ(Opts.IncludeIneligibleResults,
+ContainsItem(Results, "private_field"));
// Global items.
EXPECT_FALSE(ContainsItem(Results, "global_var"));
EXPECT_FALSE(ContainsItem(Results, GlobalFuncItemText));
@@ -889,18 +894,26 @@
}
};
- for (bool IncludeMacros : {true, false})
-for (bool IncludeGlobals : {true, false})
- for (bool IncludeBriefComments : {true, false})
-for (bool EnableSnippets : {true, false})
+ clangd::CodeCompleteOptions CCOpts;
+ for (bool IncludeMacros : {true, false}){
+CCOpts.IncludeMacros = IncludeMacros;
+for (bool IncludeGlobals : {true, false}){
+ CCOpts.IncludeGlobals = IncludeGlobals;
+ for (bool IncludeBriefComments : {true, false}){
+CCOpts.IncludeBriefComments = IncludeBriefComments;
+for (bool EnableSnippets : {true, false}){
+ CCOpts.EnableSnippets = EnableSnippets;
for (bool IncludeCodePatterns : {true, false}) {
-TestWithOpts(clangd::CodeCompleteOptions(
-/*EnableSnippets=*/EnableSnippets,
-/*IncludeCodePatterns=*/IncludeCodePatterns,
-/*IncludeMacros=*/IncludeMacros,
-/*IncludeGlobals=*/IncludeGlobals,
-/*IncludeBriefComments=*/IncludeBriefComments));
+CCOpts.IncludeCodePatterns = IncludeCodePatterns;
+for (bool IncludeIneligibleResults : {true, false}) {
+ CCOpts.IncludeIneligibleResults = IncludeIneligibleResults;
+ TestWithOpts(CCOpts);
+}
}
+}
+ }
+}
+ }
}
class ClangdThreadingTest : public ClangdVFSTest {};
Index: test/clangd/completion-qualifiers.test
===
--- test/clangd/completion-qualifiers.test
+++ test/clangd/completion-qualifiers.test
@@ -13,7 +13,7 @@
# CHECK-NEXT: "result": {
# CHECK-NEXT:"isIncomplete": false,
# CHECK-NEXT:"items": [
-# Eligible const functions are at the top of the list.
+# Eligible functions are at the top of the list.
# CHECK-NEXT: {
# CHECK-NEXT:"detail": "int",
# CHECK-NEXT:"filterText": "bar",
@@ -32,18 +32,9 @@
# CHECK-NEXT:"label": "Foo::foo() const",
# CHECK-NEXT:"sortText": "37foo"
# CHECK-NEXT: },
-# Ineligible non-const function is at the bottom of the list.
-# CHECK-NEXT: {
-# CHECK:"detail": "int",
-# CHECK:"filterText": "foo",
-# CHECK-NEXT:"insertText": "foo",
-# CHECK-NEXT:"insertTextFormat": 1,
-# CHECK-NEXT:"kind": 2,
-# CHECK-NEXT:"label": "foo() const",
-# CHECK-NEXT:"sortText": "200035foo"
-# CHECK-NEXT: }
-# CHECK-NEXT:]
-# CHECK-NEXT: }
+# Ineligible private functions are not present.
+# CHECK-NOT:"label": "foo() const",
+# CHECK:]
Content-Length: 44
{"jsonrpc":"2.0","id":4,"method":"shutdown"}
Index: test/clangd/completion-priorities.test
===
--- test/clangd/completion-priorities.test
+++ test/clangd/completion-priorities.test
@@ -61,28 +61,10 @@
# CHECK-NEXT:"kind": 2,
# CHECK-NEXT:"label": "pub()",
# CHECK-NEXT:"sortText": "34pub"
-# CHECK-NEXT: },
-# priv() and prot() are at the end of the list
-# CHECK-NEXT: {
-# CHECK:"detail": "void",
-# CHECK:"filterText": "priv",
-# CHECK-NEXT:"insertText": "priv",
-# CHECK-NEXT:"insertTextFormat": 1,
-# CHECK-NEXT:"kind": 2,
-# CHECK-NEXT:"label": "priv()",
-# CHECK-NEXT:"sortText": "200034priv"
-# CHECK-NEXT: },
-# CHECK-NEXT: {
-# CHECK-NEXT:"detail": "void",
-# CHECK-NEXT:"filterText": "prot",
-# CHECK-NEXT:"insertText": "prot",
-# CHECK-NEXT:"insertTextFormat": 1,
-# CHECK-NEXT:"kind": 2,
-# CHECK-NEXT:
[PATCH] D39836: [clangd] Drop impossible completions (unavailable or inaccessible)
sammccall updated this revision to Diff 123817.
sammccall added a comment.
This revision is now accepted and ready to land.
Add an option to allow impossible completions.
While here, remove 'helpful' constructors that take some subset of the params.
https://reviews.llvm.org/D39836
Files:
clangd/ClangdLSPServer.cpp
clangd/ClangdLSPServer.h
clangd/ClangdServer.cpp
clangd/ClangdServer.h
clangd/ClangdUnit.cpp
clangd/ClangdUnit.h
clangd/JSONExpr.cpp
clangd/JSONExpr.h
clangd/tool/ClangdMain.cpp
test/clangd/completion-priorities.test
test/clangd/completion-qualifiers.test
unittests/clangd/ClangdTests.cpp
unittests/clangd/JSONExprTests.cpp
Index: unittests/clangd/JSONExprTests.cpp
===
--- unittests/clangd/JSONExprTests.cpp
+++ unittests/clangd/JSONExprTests.cpp
@@ -15,6 +15,9 @@
namespace clang {
namespace clangd {
namespace json {
+void PrintTo(const Expr , std::ostream *OS) {
+ llvm::raw_os_ostream(*OS) << llvm::formatv("{0:2}", E);
+}
namespace {
std::string s(const Expr ) { return llvm::formatv("{0}", E).str(); }
@@ -108,6 +111,77 @@
}));
}
+TEST(JSONTest, Parse) {
+ auto Compare = [](llvm::StringRef S, Expr Expected) {
+if (auto E = parse(S)) {
+ // Compare both string forms and with operator==, in case we have bugs.
+ EXPECT_EQ(*E, Expected);
+ EXPECT_EQ(sp(*E), sp(Expected));
+} else {
+ handleAllErrors(E.takeError(), [S](const llvm::ErrorInfoBase ) {
+FAIL() << "Failed to parse JSON >>> " << S << " <<<: " << E.message();
+ });
+}
+ };
+
+ Compare(R"(true)", true);
+ Compare(R"(false)", false);
+ Compare(R"(null)", nullptr);
+
+ Compare(R"(42)", 42);
+ Compare(R"(2.5)", 2.5);
+ Compare(R"(2e50)", 2e50);
+ Compare(R"(1.2e3456789)", 1.0 / 0.0);
+
+ Compare(R"("foo")", "foo");
+ Compare(R"("\"\\\b\f\n\r\t")", "\"\\\b\f\n\r\t");
+ Compare(R"("\u")", llvm::StringRef("\0", 1));
+ Compare("\"\x7f\"", "\x7f");
+ Compare(R"("\ud801\udc37")", "\U00010437"); // UTF16 surrogate pair escape.
+ Compare("\"\xE2\x82\xAC\xF0\x9D\x84\x9E\"", "\u20ac\U0001d11e"); // UTF8
+ Compare(R"("\ud801")", "\ufffd"); // Invalid codepoint.
+
+ Compare(R"({"":0,"":0})", obj{{"", 0}});
+ Compare(R"({"obj":{},"arr":[]})", obj{{"obj", obj{}}, {"arr", {}}});
+ Compare(R"({"\n":{"\u":}})",
+ obj{{"\n", obj{
+ {llvm::StringRef("\0", 1), },
+ }}});
+ Compare("\r[\n\t] ", {});
+}
+
+TEST(JSONTest, ParseErrors) {
+ auto ExpectErr = [](llvm::StringRef Msg, llvm::StringRef S) {
+if (auto E = parse(S)) {
+ // Compare both string forms and with operator==, in case we have bugs.
+ FAIL() << "Parsed JSON >>> " << S << " <<< but wanted error: " << Msg;
+} else {
+ handleAllErrors(E.takeError(), [S, Msg](const llvm::ErrorInfoBase ) {
+EXPECT_THAT(E.message(), testing::HasSubstr(Msg)) << S;
+ });
+}
+ };
+ ExpectErr("Unexpected EOF", "");
+ ExpectErr("Unexpected EOF", "[");
+ ExpectErr("Text after end of document", "[][]");
+ ExpectErr("Text after end of document", "[][]");
+ ExpectErr("Invalid bareword", "fuzzy");
+ ExpectErr("Expected , or ]", "[2?]");
+ ExpectErr("Expected object key", "{a:2}");
+ ExpectErr("Expected : after object key", R"({"a",2})");
+ ExpectErr("Expected , or } after object property", R"({"a":2 "b":3})");
+ ExpectErr("Expected JSON value", R"([&%!])");
+ ExpectErr("Invalid number", "1e1.0");
+ ExpectErr("Unterminated string", R"("abc\"def)");
+ ExpectErr("Control character in string", "\"abc\ndef\"");
+ ExpectErr("Invalid escape sequence", R"("\030")");
+ ExpectErr("Invalid \\u escape sequence", R"("\usuck")");
+ ExpectErr("[3:3, byte=19]", R"({
+ "valid": 1,
+ invalid: 2
+})");
+}
+
} // namespace
} // namespace json
} // namespace clangd
Index: unittests/clangd/ClangdTests.cpp
===
--- unittests/clangd/ClangdTests.cpp
+++ unittests/clangd/ClangdTests.cpp
@@ -788,6 +788,8 @@
int method();
int field;
+private:
+ int private_field;
};
int test() {
@@ -828,7 +830,10 @@
// Class members. The only items that must be present in after-dor
// completion.
EXPECT_TRUE(ContainsItem(Results, MethodItemText));
+ EXPECT_TRUE(ContainsItem(Results, MethodItemText));
EXPECT_TRUE(ContainsItem(Results, "field"));
+ EXPECT_EQ(Opts.IncludeIneligibleResults,
+ContainsItem(Results, "private_field"));
// Global items.
EXPECT_FALSE(ContainsItem(Results, "global_var"));
EXPECT_FALSE(ContainsItem(Results, GlobalFuncItemText));
@@ -889,18 +894,26 @@
}
};
- for (bool IncludeMacros : {true, false})
-for (bool IncludeGlobals : {true, false})
- for (bool IncludeBriefComments : {true, false})
-for (bool EnableSnippets : {true, false})
+ clangd::CodeCompleteOptions
[PATCH] D39836: [clangd] Drop impossible completions (unavailable or inaccessible)
sammccall added a comment. In https://reviews.llvm.org/D39836#920977, @arphaman wrote: > In https://reviews.llvm.org/D39836#920587, @sammccall wrote: > > > So I probably should have started from the other end, here :-) > > > > I'd really like to make the completion retrieval and ranking more flexible. > > In particular > > > > - incorporating results from other sources (indexes: both in-memory and > > external services). > > - combining more quality signals like usage count and fuzzy-match-strength > > in a non-lexicographic-sort way The biggest difficulty in *supporting* > > unusable functions is maintaining the invariant that all unusable functions > > are ranked lower than usable ones - all code that deals with ranking has to > > deal with this special case, e.g. by making score a tuple instead of a > > single number. > > > That sounds good to me. Please keep in mind that not all clients might want > to take advantage of these things as they might have their own fuzz-match > logic Yup! My understanding of the protocol is clients will generally trigger completion after the dot, and then filter client-side *unless* the result set is incomplete. So fuzzy-match filtering/scoring would only be triggered if we limit the number of results (proposed as optional in https://reviews.llvm.org/D39852) or if completion is explicitly triggered mid-identifier (maybe we should allow turning this off too). > and external result injection. Absolutely. To be concrete, i'm thinking about three overlaid: 1. a static generated index that clangd consumes (e.g. generated by index-while-building and located on disk, or a hosted index of google's monorepo) 2. a dynamically generated index of symbols in files that clangd has seen. (Likely to be dirty with respect to 1) 3. context-sensitive completion results as today 1 would definitely be optional. 2 isn't really external... we might want to handle global symbols with 2 and not 3 as now, but that's an implementation detail. >> If the current approach of "give them a penalty" is enough, knowing that in >> the future it may lead to e.g. a very widely used but inaccessible protected >> function being ranked highly, then that seems fine to me too. A wider >> configuration space means testing is more work, but happy to live with it. >> What do you think? >> >> (With my user-hat on, configurable is fine, though I do strongly feel they >> should be off by default, and it seems unlikely many users will change the >> defaults.) > > I'd be ok with off by default, as long as it's possible to turn it on :) Sure, I'll update the patch. For these things that are (I assume) more "user preference" than project- or integration-specific, I wonder whether command-line flags are the right thing. Configuration in `~/.clangd` is a can of worms, but might be the right thing in the long-run. https://reviews.llvm.org/D39836 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D39836: [clangd] Drop impossible completions (unavailable or inaccessible)
arphaman added a comment. In https://reviews.llvm.org/D39836#920587, @sammccall wrote: > So I probably should have started from the other end, here :-) > > I'd really like to make the completion retrieval and ranking more flexible. > In particular > > - incorporating results from other sources (indexes: both in-memory and > external services). > - combining more quality signals like usage count and fuzzy-match-strength in > a non-lexicographic-sort way The biggest difficulty in *supporting* unusable > functions is maintaining the invariant that all unusable functions are ranked > lower than usable ones - all code that deals with ranking has to deal with > this special case, e.g. by making score a tuple instead of a single number. That sounds good to me. Please keep in mind that not all clients might want to take advantage of these things as they might have their own fuzz-match logic and external result injection. > If the current approach of "give them a penalty" is enough, knowing that in > the future it may lead to e.g. a very widely used but inaccessible protected > function being ranked highly, then that seems fine to me too. A wider > configuration space means testing is more work, but happy to live with it. > What do you think? > > (With my user-hat on, configurable is fine, though I do strongly feel they > should be off by default, and it seems unlikely many users will change the > defaults.) I'd be ok with off by default, as long as it's possible to turn it on :) https://reviews.llvm.org/D39836 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D39836: [clangd] Drop impossible completions (unavailable or inaccessible)
sammccall added a comment. So I probably should have started from the other end, here :-) I'd really like to make the completion retrieval and ranking more flexible. In particular - incorporating results from other sources (indexes: both in-memory and external services). - combining more quality signals like usage count and fuzzy-match-strength in a non-lexicographic-sort way The biggest difficulty in *supporting* unusable functions is maintaining the invariant that all unusable functions are ranked lower than usable ones - all code that deals with ranking has to deal with this special case, e.g. by making score a tuple instead of a single number. If the current approach of "give them a penalty" is enough, knowing that in the future it may lead to e.g. a very widely used but inaccessible protected function being ranked highly, then that seems fine to me too. A wider configuration space means testing is more work, but happy to live with it. What do you think? (With my user-hat on, configurable is fine, though I do strongly feel they should be off by default, and it seems unlikely many users will change the defaults.) https://reviews.llvm.org/D39836 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D39836: [clangd] Drop impossible completions (unavailable or inaccessible)
arphaman added a comment. I would prefer to make this behaviour configurable. https://reviews.llvm.org/D39836 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D39836: [clangd] Drop impossible completions (unavailable or inaccessible)
ilya-biryukov accepted this revision. ilya-biryukov added a comment. This revision is now accepted and ready to land. This is probably one of the things that I'd like to be configurable. In https://reviews.llvm.org/D39836#920313, @sammccall wrote: > - they bulk up tests and debugging output I'd argue that the more you put into tests, the more potential errors you'll see. E.g. if we're not showing private members, we might miss some presentation issues that only those exhibit. And you can always leave them out in tests in case you only want to test the public ones (though that's not a good idea, probably). As for debugging, I don't LSP is any good as a debugging format anyway, we probably want some different form of output. > - they complicate the scoring scheme - we *never* want them to appear above a > legal completion, so a 1-dimensional score needs special handling as here. We could always use a 2-dimensional score `(accessible, sort_score)`. I'd love to have some editor support to grey-out some items (not only inaccessible, deprecated items could have been somehow signalled in completion UI as well), probably requires some changes to LSP too and not a priority, though. > - compute: we spend time scoring them, computing their strings, writing them > over the wire, and then the client has to process them I don't think it matters much, given that there aren't many items that are inaccessible compared to the total number of completion items when it's slow (they are all class members, but the biggest portion of items come from namespaces). And after-dot completion is usually fast (and when it's slow it's because reparse is slow, not because we take too much time to process the items). That being said, I don't think there's a chance I'll argue my way through this. Let's turn them off and see if someone (besides me) complains. LGTM. https://reviews.llvm.org/D39836 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D39836: [clangd] Drop impossible completions (unavailable or inaccessible)
klimek added a comment. In https://reviews.llvm.org/D39836#920313, @sammccall wrote: > In https://reviews.llvm.org/D39836#920306, @ilya-biryukov wrote: > > > I personally think they're useful to have anyway, and they don't add much > > noise when they're at the end of completions list. > > I sometimes want to complete an item I know is there and then fix > > accessibility. Do you think they add too much noise? > > > For me the noise definitely outweighs the value, e.g. when I'm trying to see > all the available functions. There are costs beyond noise: > > - they bulk up tests and debugging output > - they complicate the scoring scheme - we *never* want them to appear above a > legal completion, so a 1-dimensional score needs special handling as here. > - compute: we spend time scoring them, computing their strings, writing them > over the wire, and then the client has to process them I do think they are useful to me, but I don't know what the percentages of folks who want them vs. not are, and whether people have actually tried multiple schemes. I think it's more likely that we get bug reports to put them in if we don't show them than the other way round, so I think taking them out is a good way to get that feedback :) https://reviews.llvm.org/D39836 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D39836: [clangd] Drop impossible completions (unavailable or inaccessible)
sammccall added a comment. In https://reviews.llvm.org/D39836#920306, @ilya-biryukov wrote: > I personally think they're useful to have anyway, and they don't add much > noise when they're at the end of completions list. > I sometimes want to complete an item I know is there and then fix > accessibility. Do you think they add too much noise? For me the noise definitely outweighs the value, e.g. when I'm trying to see all the available functions. There are costs beyond noise: - they bulk up tests and debugging output - they complicate the scoring scheme - we *never* want them to appear above a legal completion, so a 1-dimensional score needs special handling as here. - compute: we spend time scoring them, computing their strings, writing them over the wire, and then the client has to process them https://reviews.llvm.org/D39836 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D39836: [clangd] Drop impossible completions (unavailable or inaccessible)
ilya-biryukov added a subscriber: klimek. ilya-biryukov added a comment. I personally think they're useful to have anyway, and they don't add much noise when they're at the end of completions list. I sometimes want to complete an item I know is there and then fix accessibility. Do you think they add too much noise? I think I'm almost alone in this camp, though. (I remember @klimek saying he thinks they're sometimes useful too, but I may be mistaken) https://reviews.llvm.org/D39836 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D39836: [clangd] Drop impossible completions (unavailable or inaccessible)
sammccall created this revision.
(There must be some reason why https://reviews.llvm.org/D38077 didn't just do
this, but I don't get it!)
https://reviews.llvm.org/D39836
Files:
clangd/ClangdUnit.cpp
test/clangd/completion-priorities.test
test/clangd/completion-qualifiers.test
Index: test/clangd/completion-qualifiers.test
===
--- test/clangd/completion-qualifiers.test
+++ test/clangd/completion-qualifiers.test
@@ -11,7 +11,7 @@
# CHECK: "id": 2,
# CHECK-NEXT: "jsonrpc": "2.0",
# CHECK-NEXT: "result": [
-# Eligible const functions are at the top of the list.
+# Eligible functions are at the top of the list.
# CHECK-NEXT:{
# CHECK-NEXT: "detail": "int",
# CHECK-NEXT: "filterText": "bar",
@@ -30,17 +30,9 @@
# CHECK-NEXT: "label": "Foo::foo() const",
# CHECK-NEXT: "sortText": "37foo"
# CHECK-NEXT:},
-# Ineligible non-const function is at the bottom of the list.
-# CHECK-NEXT:{
-# CHECK: "detail": "int",
-# CHECK: "filterText": "foo",
-# CHECK-NEXT: "insertText": "foo",
-# CHECK-NEXT: "insertTextFormat": 1,
-# CHECK-NEXT: "kind": 2,
-# CHECK-NEXT: "label": "foo() const",
-# CHECK-NEXT: "sortText": "200035foo"
-# CHECK-NEXT:}
-# CHECK-NEXT: ]
+# Ineligible private functions are not present.
+# CHECK-NOT: "label": "foo() const",
+# CHECK: ]
Content-Length: 44
{"jsonrpc":"2.0","id":4,"method":"shutdown"}
Index: test/clangd/completion-priorities.test
===
--- test/clangd/completion-priorities.test
+++ test/clangd/completion-priorities.test
@@ -57,27 +57,10 @@
# CHECK-NEXT: "kind": 2,
# CHECK-NEXT: "label": "pub()",
# CHECK-NEXT: "sortText": "34pub"
-# CHECK-NEXT:},
-# priv() and prot() are at the end of the list
-# CHECK-NEXT:{
-# CHECK: "detail": "void",
-# CHECK: "filterText": "priv",
-# CHECK-NEXT: "insertText": "priv",
-# CHECK-NEXT: "insertTextFormat": 1,
-# CHECK-NEXT: "kind": 2,
-# CHECK-NEXT: "label": "priv()",
-# CHECK-NEXT: "sortText": "200034priv"
-# CHECK-NEXT:},
-# CHECK-NEXT:{
-# CHECK-NEXT: "detail": "void",
-# CHECK-NEXT: "filterText": "prot",
-# CHECK-NEXT: "insertText": "prot",
-# CHECK-NEXT: "insertTextFormat": 1,
-# CHECK-NEXT: "kind": 2,
-# CHECK-NEXT: "label": "prot()",
-# CHECK-NEXT: "sortText": "200034prot"
# CHECK-NEXT:}
-# CHECK-NEXT: ]
+# CHECK-NOT: "label": "priv()",
+# CHECK-NOT: "label": "prot()",
+# CHECK: ]
Content-Length: 58
{"jsonrpc":"2.0","id":4,"method":"shutdown","params":null}
Index: clangd/ClangdUnit.cpp
===
--- clangd/ClangdUnit.cpp
+++ clangd/ClangdUnit.cpp
@@ -383,6 +383,9 @@
Items.reserve(NumResults);
for (unsigned I = 0; I < NumResults; ++I) {
auto = Results[I];
+ if (Result.Availability == CXAvailability_NotAvailable ||
+ Result.Availability == CXAvailability_NotAccessible)
+continue;
const auto *CCS = Result.CreateCodeCompletionString(
S, Context, *Allocator, CCTUInfo,
CodeCompleteOpts.IncludeBriefComments);
@@ -428,23 +431,8 @@
// Fill in the sortText of the CompletionItem.
assert(Score <= 9 && "Expecting code completion result "
"priority to have at most 5-digits");
-
-const int Penalty = 10;
-switch (static_cast(CCS.getAvailability())) {
-case CXAvailability_Available:
- // No penalty.
- break;
-case CXAvailability_Deprecated:
- Score += Penalty;
- break;
-case CXAvailability_NotAccessible:
- Score += 2 * Penalty;
- break;
-case CXAvailability_NotAvailable:
- Score += 3 * Penalty;
- break;
-}
-
+if (CCS.getAvailability() == CXAvailability_Deprecated)
+ Score += 10;
return Score;
}
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
