sammccall updated this revision to Diff 264721.
sammccall added a comment.
Address comments
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D79456/new/
https://reviews.llvm.org/D79456
Files:
clang-tools-extra/clangd/ClangdLSPServer.cpp
clang-tools-extra/clangd/CodeComplete.cpp
clang-tools-extra/clangd/CodeComplete.h
clang-tools-extra/clangd/test/initialize-params.test
clang-tools-extra/clangd/unittests/CodeCompleteTests.cpp
Index: clang-tools-extra/clangd/unittests/CodeCompleteTests.cpp
===================================================================
--- clang-tools-extra/clangd/unittests/CodeCompleteTests.cpp
+++ clang-tools-extra/clangd/unittests/CodeCompleteTests.cpp
@@ -25,6 +25,7 @@
#include "clang/Tooling/CompilationDatabase.h"
#include "llvm/Support/Error.h"
#include "llvm/Support/Path.h"
+#include "llvm/Testing/Support/Annotations.h"
#include "llvm/Testing/Support/Error.h"
#include "gmock/gmock.h"
#include "gtest/gtest.h"
@@ -2828,6 +2829,34 @@
ElementsAre(AllOf(Qualifier(""), Scope("a::"))));
}
+TEST(AllowImplicitCompletion, All) {
+ const char *Yes[] = {
+ "foo.^bar",
+ "foo->^bar",
+ "foo::^bar",
+ " # include <^foo.h>",
+ "#import <foo/^bar.h>",
+ "#include_next \"^",
+ };
+ const char *No[] = {
+ "foo>^bar",
+ "foo:^bar",
+ "foo\n^bar",
+ "#include <foo.h> //^",
+ "#include \"foo.h\"^",
+ "#error <^",
+ "#<^",
+ };
+ for (const char *Test : Yes) {
+ llvm::Annotations A(Test);
+ EXPECT_TRUE(allowImplicitCompletion(A.code(), A.point())) << Test;
+ }
+ for (const char *Test : No) {
+ llvm::Annotations A(Test);
+ EXPECT_FALSE(allowImplicitCompletion(A.code(), A.point())) << Test;
+ }
+}
+
} // namespace
} // namespace clangd
} // namespace clang
Index: clang-tools-extra/clangd/test/initialize-params.test
===================================================================
--- clang-tools-extra/clangd/test/initialize-params.test
+++ clang-tools-extra/clangd/test/initialize-params.test
@@ -11,8 +11,11 @@
# CHECK-NEXT: "resolveProvider": false,
# CHECK-NEXT: "triggerCharacters": [
# CHECK-NEXT: ".",
+# CHECK-NEXT: "<",
# CHECK-NEXT: ">",
-# CHECK-NEXT: ":"
+# CHECK-NEXT: ":",
+# CHECK-NEXT: "\"",
+# CHECK-NEXT: "/"
# CHECK-NEXT: ]
# CHECK-NEXT: },
# CHECK-NEXT: "declarationProvider": true,
Index: clang-tools-extra/clangd/CodeComplete.h
===================================================================
--- clang-tools-extra/clangd/CodeComplete.h
+++ clang-tools-extra/clangd/CodeComplete.h
@@ -314,6 +314,10 @@
CompletionPrefix guessCompletionPrefix(llvm::StringRef Content,
unsigned Offset);
+// Whether it makes sense to complete at the point based on typed characters.
+// For instance, we implicitly trigger at `a->^` but not at `a>^`.
+bool allowImplicitCompletion(llvm::StringRef Content, unsigned Offset);
+
} // namespace clangd
} // namespace clang
Index: clang-tools-extra/clangd/CodeComplete.cpp
===================================================================
--- clang-tools-extra/clangd/CodeComplete.cpp
+++ clang-tools-extra/clangd/CodeComplete.cpp
@@ -1912,5 +1912,43 @@
return OS;
}
+// Heuristically detect whether the `Line` is an unterminated include filename.
+bool isIncludeFile(llvm::StringRef Line) {
+ Line = Line.ltrim();
+ if (!Line.consume_front("#"))
+ return false;
+ Line = Line.ltrim();
+ if (!(Line.consume_front("include_next") || Line.consume_front("include") ||
+ Line.consume_front("import")))
+ return false;
+ Line = Line.ltrim();
+ if (Line.consume_front("<"))
+ return Line.count('>') == 0;
+ if (Line.consume_front("\""))
+ return Line.count('"') == 0;
+ return false;
+}
+
+bool allowImplicitCompletion(llvm::StringRef Content, unsigned Offset) {
+ // Look at last line before completion point only.
+ Content = Content.take_front(Offset);
+ auto Pos = Content.rfind('\n');
+ if (Pos != llvm::StringRef::npos)
+ Content = Content.substr(Pos + 1);
+
+ // Complete after scope operators.
+ if (Content.endswith(".") || Content.endswith("->") || Content.endswith("::"))
+ return true;
+ // Complete after `#include <` and #include `<foo/`.
+ if ((Content.endswith("<") || Content.endswith("\"") ||
+ Content.endswith("/")) &&
+ isIncludeFile(Content))
+ return true;
+
+ // Complete words. Give non-ascii characters the benefit of the doubt.
+ return !Content.empty() &&
+ (isIdentifierBody(Content.back()) || !llvm::isASCII(Content.back()));
+}
+
} // namespace clangd
} // namespace clang
Index: clang-tools-extra/clangd/ClangdLSPServer.cpp
===================================================================
--- clang-tools-extra/clangd/ClangdLSPServer.cpp
+++ clang-tools-extra/clangd/ClangdLSPServer.cpp
@@ -7,6 +7,7 @@
//===----------------------------------------------------------------------===//
#include "ClangdLSPServer.h"
+#include "CodeComplete.h"
#include "Diagnostics.h"
#include "DraftStore.h"
#include "GlobalCompilationDatabase.h"
@@ -593,9 +594,8 @@
llvm::json::Object{
{"allCommitCharacters", " \t()[]{}<>:;,+-/*%^&#?.=\"'|"},
{"resolveProvider", false},
- // We do extra checks for '>' and ':' in completion to only
- // trigger on '->' and '::'.
- {"triggerCharacters", {".", ">", ":"}},
+ // We do extra checks, e.g. that > is part of ->.
+ {"triggerCharacters", {".", "<", ">", ":", "\"", "/"}},
}},
{"semanticTokensProvider",
llvm::json::Object{
@@ -1425,23 +1425,19 @@
return FixItsIter->second;
}
+// A completion request is sent when the user types '>' or ':', but we only
+// want to trigger on '->' and '::'. We check the preceeding text to make
+// sure it matches what we expected.
+// Running the lexer here would be more robust (e.g. we can detect comments
+// and avoid triggering completion there), but we choose to err on the side
+// of simplicity here.
bool ClangdLSPServer::shouldRunCompletion(
const CompletionParams &Params) const {
- llvm::StringRef Trigger = Params.context.triggerCharacter;
- if (Params.context.triggerKind != CompletionTriggerKind::TriggerCharacter ||
- (Trigger != ">" && Trigger != ":"))
+ if (Params.context.triggerKind != CompletionTriggerKind::TriggerCharacter)
return true;
-
auto Code = DraftMgr.getDraft(Params.textDocument.uri.file());
if (!Code)
return true; // completion code will log the error for untracked doc.
-
- // A completion request is sent when the user types '>' or ':', but we only
- // want to trigger on '->' and '::'. We check the preceeding character to make
- // sure it matches what we expected.
- // Running the lexer here would be more robust (e.g. we can detect comments
- // and avoid triggering completion there), but we choose to err on the side
- // of simplicity here.
auto Offset = positionToOffset(Code->Contents, Params.position,
/*AllowColumnsBeyondLineLength=*/false);
if (!Offset) {
@@ -1449,15 +1445,7 @@
Params.position, Params.textDocument.uri.file());
return true;
}
- if (*Offset < 2)
- return false;
-
- if (Trigger == ">")
- return Code->Contents[*Offset - 2] == '-'; // trigger only on '->'.
- if (Trigger == ":")
- return Code->Contents[*Offset - 2] == ':'; // trigger only on '::'.
- assert(false && "unhandled trigger character");
- return true;
+ return allowImplicitCompletion(Code->Contents, *Offset);
}
void ClangdLSPServer::onHighlightingsReady(
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
