[PATCH] D113765: [clangd] Fix function-arg-placeholder suppression with macros.
This revision was automatically updated to reflect the committed changes.
Closed by commit rG8ac9d2ae5839: [clangd] Fix function-arg-placeholder
suppression with macros. (authored by sammccall).
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D113765/new/
https://reviews.llvm.org/D113765
Files:
clang-tools-extra/clangd/CodeComplete.cpp
clang-tools-extra/clangd/tool/ClangdMain.cpp
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
@@ -1662,6 +1662,9 @@
// Overload with bool
int a(bool);
int b(float);
+
+ X(int);
+ X(float);
};
int GFuncC(int);
int GFuncD(int);
@@ -1671,6 +1674,10 @@
EXPECT_THAT(completions(Context + "int y = X().^", {}, Opts).Completions,
UnorderedElementsAre(Labeled("a(â¦)"), Labeled("b(float)")));
+ // Constructor completions are bundled.
+ EXPECT_THAT(completions(Context + "X z = X^", {}, Opts).Completions,
+ UnorderedElementsAre(Labeled("X"), Labeled("X(â¦)")));
+
// Non-member completions are bundled, including index+sema.
Symbol NoArgsGFunc = func("GFuncC");
EXPECT_THAT(
@@ -2323,6 +2330,15 @@
UnorderedElementsAre(AllOf(Named("foo_class"), SnippetSuffix("<$0>")),
AllOf(Named("foo_alias"), SnippetSuffix("<$0>";
}
+ {
+auto Results = completions(
+R"cpp(
+ #define FOO(x, y) x##f
+ FO^ )cpp",
+{}, Opts);
+EXPECT_THAT(Results.Completions, UnorderedElementsAre(AllOf(
+ Named("FOO"), SnippetSuffix("($0)";
+ }
}
TEST(CompletionTest, SuggestOverrides) {
@@ -3170,6 +3186,7 @@
clangd::CodeCompleteOptions Opts;
Opts.EnableSnippets = true;
std::string Context = R"cpp(
+#define MACRO(x)
int foo(int A);
int bar();
struct Object {
@@ -3217,6 +3234,9 @@
Contains(AllOf(Labeled("Container(int Size)"),
SnippetSuffix(""),
Kind(CompletionItemKind::Constructor;
+ EXPECT_THAT(completions(Context + "MAC^(2)", {}, Opts).Completions,
+ Contains(AllOf(Labeled("MACRO(x)"), SnippetSuffix(""),
+ Kind(CompletionItemKind::Text;
}
TEST(CompletionTest, NoCrashDueToMacroOrdering) {
Index: clang-tools-extra/clangd/tool/ClangdMain.cpp
===
--- clang-tools-extra/clangd/tool/ClangdMain.cpp
+++ clang-tools-extra/clangd/tool/ClangdMain.cpp
@@ -233,7 +233,6 @@
"function calls. When enabled, completions also contain "
"placeholders for method parameters"),
init(CodeCompleteOptions().EnableFunctionArgSnippets),
-Hidden,
};
opt HeaderInsertion{
Index: clang-tools-extra/clangd/CodeComplete.cpp
===
--- clang-tools-extra/clangd/CodeComplete.cpp
+++ clang-tools-extra/clangd/CodeComplete.cpp
@@ -466,32 +466,27 @@
// FIXME(ibiryukov): sometimes add template arguments to a snippet, e.g.
// we need to complete 'forward<$1>($0)'.
return "($0)";
-// Suppress function argument snippets cursor is followed by left
-// parenthesis (and potentially arguments) or if there are potentially
-// template arguments. There are cases where it would be wrong (e.g. next
-// '<' token is a comparison rather than template argument list start) but
-// it is less common and suppressing snippet provides better UX.
-if (Completion.Kind == CompletionItemKind::Function ||
-Completion.Kind == CompletionItemKind::Method ||
-Completion.Kind == CompletionItemKind::Constructor) {
- // If there is a potential template argument list, drop snippet and just
- // complete symbol name. Ideally, this could generate an edit that would
- // paste function arguments after template argument list but it would be
- // complicated. Example:
- //
- // fu^ -> function
+
+bool MayHaveArgList = Completion.Kind == CompletionItemKind::Function ||
+ Completion.Kind == CompletionItemKind::Method ||
+ Completion.Kind == CompletionItemKind::Constructor ||
+ Completion.Kind == CompletionItemKind::Text /*Macro*/;
+// If likely arg list already exists, don't add new parens & placeholders.
+// Snippet: function(int x, int y)
+// func^(1,2) -> function(1, 2)
+// NOT function(int x, int y)(1, 2)
+if (MayHaveArgList) {
+ // Check for a template argument list in the code.
+ // Snippet: function(int x)
+ // fu^(1) -> function(1)
if (NextTokenKind ==
[PATCH] D113765: [clangd] Fix function-arg-placeholder suppression with macros.
kadircet accepted this revision.
kadircet added inline comments.
This revision is now accepted and ready to land.
Comment at: clang-tools-extra/clangd/CodeComplete.cpp:517
+Completion.Kind == CompletionItemKind::Method ||
+Completion.Kind == CompletionItemKind::Text /*Macro*/) {
// Functions snippets can be of 2 types:
sammccall wrote:
> kadircet wrote:
> > again while here, maybe introduce constructors into this condition. (Even
> > better, what about a `bool isFunctionLikeCompletion(const CompletionItem&)`
> > that we can use both here and above?)
> Done, though I didn't want to extract the variable too far away as there are
> some wrinkles (macros may not be function-like, template args...) that we can
> get away without resolving here.
thanks! yes that makes sense.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D113765/new/
https://reviews.llvm.org/D113765
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D113765: [clangd] Fix function-arg-placeholder suppression with macros.
sammccall updated this revision to Diff 386873.
sammccall marked 2 inline comments as done.
sammccall added a comment.
Address review comments.
While here, condense some long comments that repeated the code and seemed
confusing.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D113765/new/
https://reviews.llvm.org/D113765
Files:
clang-tools-extra/clangd/CodeComplete.cpp
clang-tools-extra/clangd/tool/ClangdMain.cpp
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
@@ -1662,6 +1662,9 @@
// Overload with bool
int a(bool);
int b(float);
+
+ X(int);
+ X(float);
};
int GFuncC(int);
int GFuncD(int);
@@ -1671,6 +1674,10 @@
EXPECT_THAT(completions(Context + "int y = X().^", {}, Opts).Completions,
UnorderedElementsAre(Labeled("a(â¦)"), Labeled("b(float)")));
+ // Constructor completions are bundled.
+ EXPECT_THAT(completions(Context + "X z = X^", {}, Opts).Completions,
+ UnorderedElementsAre(Labeled("X"), Labeled("X(â¦)")));
+
// Non-member completions are bundled, including index+sema.
Symbol NoArgsGFunc = func("GFuncC");
EXPECT_THAT(
@@ -2318,6 +2325,15 @@
UnorderedElementsAre(AllOf(Named("foo_class"), SnippetSuffix("<$0>")),
AllOf(Named("foo_alias"), SnippetSuffix("<$0>";
}
+ {
+auto Results = completions(
+R"cpp(
+ #define FOO(x, y) x##f
+ FO^ )cpp",
+{}, Opts);
+EXPECT_THAT(Results.Completions, UnorderedElementsAre(AllOf(
+ Named("FOO"), SnippetSuffix("($0)";
+ }
}
TEST(CompletionTest, SuggestOverrides) {
@@ -3165,6 +3181,7 @@
clangd::CodeCompleteOptions Opts;
Opts.EnableSnippets = true;
std::string Context = R"cpp(
+#define MACRO(x)
int foo(int A);
int bar();
struct Object {
@@ -3212,6 +3229,9 @@
Contains(AllOf(Labeled("Container(int Size)"),
SnippetSuffix(""),
Kind(CompletionItemKind::Constructor;
+ EXPECT_THAT(completions(Context + "MAC^(2)", {}, Opts).Completions,
+ Contains(AllOf(Labeled("MACRO(x)"), SnippetSuffix(""),
+ Kind(CompletionItemKind::Text;
}
TEST(CompletionTest, NoCrashDueToMacroOrdering) {
Index: clang-tools-extra/clangd/tool/ClangdMain.cpp
===
--- clang-tools-extra/clangd/tool/ClangdMain.cpp
+++ clang-tools-extra/clangd/tool/ClangdMain.cpp
@@ -233,7 +233,6 @@
"function calls. When enabled, completions also contain "
"placeholders for method parameters"),
init(CodeCompleteOptions().EnableFunctionArgSnippets),
-Hidden,
};
opt HeaderInsertion{
Index: clang-tools-extra/clangd/CodeComplete.cpp
===
--- clang-tools-extra/clangd/CodeComplete.cpp
+++ clang-tools-extra/clangd/CodeComplete.cpp
@@ -466,32 +466,27 @@
// FIXME(ibiryukov): sometimes add template arguments to a snippet, e.g.
// we need to complete 'forward<$1>($0)'.
return "($0)";
-// Suppress function argument snippets cursor is followed by left
-// parenthesis (and potentially arguments) or if there are potentially
-// template arguments. There are cases where it would be wrong (e.g. next
-// '<' token is a comparison rather than template argument list start) but
-// it is less common and suppressing snippet provides better UX.
-if (Completion.Kind == CompletionItemKind::Function ||
-Completion.Kind == CompletionItemKind::Method ||
-Completion.Kind == CompletionItemKind::Constructor) {
- // If there is a potential template argument list, drop snippet and just
- // complete symbol name. Ideally, this could generate an edit that would
- // paste function arguments after template argument list but it would be
- // complicated. Example:
- //
- // fu^ -> function
+
+bool MayHaveArgList = Completion.Kind == CompletionItemKind::Function ||
+ Completion.Kind == CompletionItemKind::Method ||
+ Completion.Kind == CompletionItemKind::Constructor ||
+ Completion.Kind == CompletionItemKind::Text /*Macro*/;
+// If likely arg list already exists, don't add new parens & placeholders.
+// Snippet: function(int x, int y)
+// func^(1,2) -> function(1, 2)
+// NOT function(int x, int y)(1, 2)
+if (MayHaveArgList) {
+ // Check for a template argument list in the code.
+ // Snippet: function(int x)
+ // fu^(1) ->
[PATCH] D113765: [clangd] Fix function-arg-placeholder suppression with macros.
sammccall added inline comments.
Comment at: clang-tools-extra/clangd/CodeComplete.cpp:517
+Completion.Kind == CompletionItemKind::Method ||
+Completion.Kind == CompletionItemKind::Text /*Macro*/) {
// Functions snippets can be of 2 types:
kadircet wrote:
> again while here, maybe introduce constructors into this condition. (Even
> better, what about a `bool isFunctionLikeCompletion(const CompletionItem&)`
> that we can use both here and above?)
Done, though I didn't want to extract the variable too far away as there are
some wrinkles (macros may not be function-like, template args...) that we can
get away without resolving here.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D113765/new/
https://reviews.llvm.org/D113765
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D113765: [clangd] Fix function-arg-placeholder suppression with macros.
kadircet added inline comments.
Comment at: clang-tools-extra/clangd/CodeComplete.cpp:476
Completion.Kind == CompletionItemKind::Method ||
Completion.Kind == CompletionItemKind::Constructor) {
// If there is a potential template argument list, drop snippet and just
while here, i suppose we should perform drop arguments and parentheses magic
for macros as well?
Comment at: clang-tools-extra/clangd/CodeComplete.cpp:517
+Completion.Kind == CompletionItemKind::Method ||
+Completion.Kind == CompletionItemKind::Text /*Macro*/) {
// Functions snippets can be of 2 types:
again while here, maybe introduce constructors into this condition. (Even
better, what about a `bool isFunctionLikeCompletion(const CompletionItem&)`
that we can use both here and above?)
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D113765/new/
https://reviews.llvm.org/D113765
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D113765: [clangd] Fix function-arg-placeholder suppression with macros.
sammccall created this revision.
sammccall added a reviewer: kadircet.
Herald added subscribers: usaxena95, arphaman.
sammccall requested review of this revision.
Herald added subscribers: cfe-commits, MaskRay, ilya-biryukov.
Herald added a project: clang-tools-extra.
While here, unhide function-arg-placeholders flag. It's reasonable to want and
maybe we should consider making it default.
Repository:
rG LLVM Github Monorepo
https://reviews.llvm.org/D113765
Files:
clang-tools-extra/clangd/CodeComplete.cpp
clang-tools-extra/clangd/tool/ClangdMain.cpp
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
@@ -2318,6 +2318,15 @@
UnorderedElementsAre(AllOf(Named("foo_class"), SnippetSuffix("<$0>")),
AllOf(Named("foo_alias"),
SnippetSuffix("<$0>";
}
+ {
+auto Results = completions(
+R"cpp(
+ #define FOO(x, y) x##f
+ FO^ )cpp",
+{}, Opts);
+EXPECT_THAT(Results.Completions, UnorderedElementsAre(AllOf(
+ Named("FOO"),
SnippetSuffix("($0)";
+ }
}
TEST(CompletionTest, SuggestOverrides) {
Index: clang-tools-extra/clangd/tool/ClangdMain.cpp
===
--- clang-tools-extra/clangd/tool/ClangdMain.cpp
+++ clang-tools-extra/clangd/tool/ClangdMain.cpp
@@ -233,7 +233,6 @@
"function calls. When enabled, completions also contain "
"placeholders for method parameters"),
init(CodeCompleteOptions().EnableFunctionArgSnippets),
-Hidden,
};
opt HeaderInsertion{
Index: clang-tools-extra/clangd/CodeComplete.cpp
===
--- clang-tools-extra/clangd/CodeComplete.cpp
+++ clang-tools-extra/clangd/CodeComplete.cpp
@@ -513,7 +513,8 @@
if (Snippet->empty())
return "";
if (Completion.Kind == CompletionItemKind::Function ||
-Completion.Kind == CompletionItemKind::Method) {
+Completion.Kind == CompletionItemKind::Method ||
+Completion.Kind == CompletionItemKind::Text /*Macro*/) {
// Functions snippets can be of 2 types:
// - containing only function arguments, e.g.
// foo(${1:int p1}, ${2:int p2});
Index: clang-tools-extra/clangd/unittests/CodeCompleteTests.cpp
===
--- clang-tools-extra/clangd/unittests/CodeCompleteTests.cpp
+++ clang-tools-extra/clangd/unittests/CodeCompleteTests.cpp
@@ -2318,6 +2318,15 @@
UnorderedElementsAre(AllOf(Named("foo_class"), SnippetSuffix("<$0>")),
AllOf(Named("foo_alias"), SnippetSuffix("<$0>";
}
+ {
+auto Results = completions(
+R"cpp(
+ #define FOO(x, y) x##f
+ FO^ )cpp",
+{}, Opts);
+EXPECT_THAT(Results.Completions, UnorderedElementsAre(AllOf(
+ Named("FOO"), SnippetSuffix("($0)";
+ }
}
TEST(CompletionTest, SuggestOverrides) {
Index: clang-tools-extra/clangd/tool/ClangdMain.cpp
===
--- clang-tools-extra/clangd/tool/ClangdMain.cpp
+++ clang-tools-extra/clangd/tool/ClangdMain.cpp
@@ -233,7 +233,6 @@
"function calls. When enabled, completions also contain "
"placeholders for method parameters"),
init(CodeCompleteOptions().EnableFunctionArgSnippets),
-Hidden,
};
opt HeaderInsertion{
Index: clang-tools-extra/clangd/CodeComplete.cpp
===
--- clang-tools-extra/clangd/CodeComplete.cpp
+++ clang-tools-extra/clangd/CodeComplete.cpp
@@ -513,7 +513,8 @@
if (Snippet->empty())
return "";
if (Completion.Kind == CompletionItemKind::Function ||
-Completion.Kind == CompletionItemKind::Method) {
+Completion.Kind == CompletionItemKind::Method ||
+Completion.Kind == CompletionItemKind::Text /*Macro*/) {
// Functions snippets can be of 2 types:
// - containing only function arguments, e.g.
// foo(${1:int p1}, ${2:int p2});
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
