[PATCH] D137040: [clangd] Add heuristic for dropping snippet when completing member function pointer

[Tom Praschan via Phabricator via cfe-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rG0e00611cbc2b: [clangd] Add heuristic for dropping snippet 
when completing member function… (authored by tom-anders).

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D137040/new/

https://reviews.llvm.org/D137040

Files:
  clang-tools-extra/clangd/CodeComplete.cpp
  clang-tools-extra/clangd/unittests/CodeCompleteTests.cpp
  clang/include/clang/Sema/CodeCompleteConsumer.h
  clang/lib/Sema/SemaCodeComplete.cpp
  clang/unittests/Sema/CodeCompleteTest.cpp

Index: clang/unittests/Sema/CodeCompleteTest.cpp
===
--- clang/unittests/Sema/CodeCompleteTest.cpp
+++ clang/unittests/Sema/CodeCompleteTest.cpp
@@ -23,6 +23,8 @@
 
 using namespace clang;
 using namespace clang::tooling;
+using ::testing::AllOf;
+using ::testing::Contains;
 using ::testing::Each;
 using ::testing::UnorderedElementsAre;
 
@@ -36,6 +38,51 @@
   std::string PtrDiffType;
 };
 
+struct CompletedFunctionDecl {
+  std::string Name;
+  bool IsStatic;
+  bool CanBeCall;
+};
+MATCHER_P(named, name, "") { return arg.Name == name; }
+MATCHER_P(isStatic, value, "") { return arg.IsStatic == value; }
+MATCHER_P(canBeCall, value, "") { return arg.CanBeCall == value; }
+
+class SaveCompletedFunctions : public CodeCompleteConsumer {
+public:
+  SaveCompletedFunctions(std::vector )
+  : CodeCompleteConsumer(/*CodeCompleteOpts=*/{}),
+CompletedFuncDecls(CompletedFuncDecls),
+CCTUInfo(std::make_shared()) {}
+
+  void ProcessCodeCompleteResults(Sema , CodeCompletionContext Context,
+  CodeCompletionResult *Results,
+  unsigned NumResults) override {
+for (unsigned I = 0; I < NumResults; ++I) {
+  auto R = Results[I];
+  if (R.Kind == CodeCompletionResult::RK_Declaration) {
+if (const auto *FD = llvm::dyn_cast(R.getDeclaration())) {
+  CompletedFunctionDecl D;
+  D.Name = FD->getNameAsString();
+  D.CanBeCall = R.FunctionCanBeCall;
+  D.IsStatic = FD->isStatic();
+  CompletedFuncDecls.emplace_back(std::move(D));
+}
+  }
+}
+  }
+
+private:
+  CodeCompletionAllocator () override {
+return CCTUInfo.getAllocator();
+  }
+
+  CodeCompletionTUInfo () override { return CCTUInfo; }
+
+  std::vector 
+
+  CodeCompletionTUInfo CCTUInfo;
+};
+
 class VisitedContextFinder : public CodeCompleteConsumer {
 public:
   VisitedContextFinder(CompletionContext )
@@ -74,19 +121,19 @@
 
 class CodeCompleteAction : public SyntaxOnlyAction {
 public:
-  CodeCompleteAction(ParsedSourceLocation P, CompletionContext )
-  : CompletePosition(std::move(P)), ResultCtx(ResultCtx) {}
+  CodeCompleteAction(ParsedSourceLocation P, CodeCompleteConsumer *Consumer)
+  : CompletePosition(std::move(P)), Consumer(Consumer) {}
 
   bool BeginInvocation(CompilerInstance ) override {
 CI.getFrontendOpts().CodeCompletionAt = CompletePosition;
-CI.setCodeCompletionConsumer(new VisitedContextFinder(ResultCtx));
+CI.setCodeCompletionConsumer(Consumer);
 return true;
   }
 
 private:
   // 1-based code complete position ;
   ParsedSourceLocation CompletePosition;
-  CompletionContext 
+  CodeCompleteConsumer *Consumer;
 };
 
 ParsedSourceLocation offsetToPosition(llvm::StringRef Code, size_t Offset) {
@@ -103,7 +150,7 @@
   CompletionContext ResultCtx;
   clang::tooling::runToolOnCodeWithArgs(
   std::make_unique(offsetToPosition(Code, Offset),
-   ResultCtx),
+   new VisitedContextFinder(ResultCtx)),
   Code, {"-std=c++11"}, TestCCName);
   return ResultCtx;
 }
@@ -129,6 +176,69 @@
   return Types;
 }
 
+std::vector
+CollectCompletedFunctions(StringRef Code, std::size_t Point) {
+  std::vector Result;
+  clang::tooling::runToolOnCodeWithArgs(
+  std::make_unique(offsetToPosition(Code, Point),
+   new SaveCompletedFunctions(Result)),
+  Code, {"-std=c++11"}, TestCCName);
+  return Result;
+}
+
+TEST(SemaCodeCompleteTest, FunctionCanBeCall) {
+  llvm::Annotations Code(R"cpp(
+struct Foo {
+  static int staticMethod();
+  int method() const;
+  Foo() {
+this->$canBeCall^
+$canBeCall^
+Foo::$canBeCall^
+  }
+};
+
+struct Derived : Foo {
+  Derived() {
+Foo::$canBeCall^
+  }
+};
+
+struct OtherClass {
+  OtherClass() {
+Foo f;
+f.$canBeCall^
+::$cannotBeCall^
+  }
+};
+
+int main() {
+  Foo f;
+  f.$canBeCall^
+  ::$cannotBeCall^
+}
+)cpp");
+
+  for (const auto  : Code.points("canBeCall")) {
+auto Results = CollectCompletedFunctions(Code.code(), P);
+EXPECT_THAT(Results, 

[PATCH] D137040: [clangd] Add heuristic for dropping snippet when completing member function pointer

[Tom Praschan via Phabricator via cfe-commits]

tom-anders added a comment.

In D137040#3924115 , @nridge wrote:

> Thanks!

Thanks for the review!


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D137040/new/

https://reviews.llvm.org/D137040

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D137040: [clangd] Add heuristic for dropping snippet when completing member function pointer

[Nathan Ridge via Phabricator via cfe-commits]

nridge accepted this revision.
nridge added a comment.
This revision is now accepted and ready to land.

Thanks!


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D137040/new/

https://reviews.llvm.org/D137040

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D137040: [clangd] Add heuristic for dropping snippet when completing member function pointer

[Tom Praschan via Phabricator via cfe-commits]

tom-anders updated this revision to Diff 474990.
tom-anders added a comment.

Clean up test


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D137040/new/

https://reviews.llvm.org/D137040

Files:
  clang-tools-extra/clangd/CodeComplete.cpp
  clang-tools-extra/clangd/unittests/CodeCompleteTests.cpp
  clang/include/clang/Sema/CodeCompleteConsumer.h
  clang/lib/Sema/SemaCodeComplete.cpp
  clang/unittests/Sema/CodeCompleteTest.cpp

Index: clang/unittests/Sema/CodeCompleteTest.cpp
===
--- clang/unittests/Sema/CodeCompleteTest.cpp
+++ clang/unittests/Sema/CodeCompleteTest.cpp
@@ -23,6 +23,8 @@
 
 using namespace clang;
 using namespace clang::tooling;
+using ::testing::AllOf;
+using ::testing::Contains;
 using ::testing::Each;
 using ::testing::UnorderedElementsAre;
 
@@ -36,6 +38,51 @@
   std::string PtrDiffType;
 };
 
+struct CompletedFunctionDecl {
+  std::string Name;
+  bool IsStatic;
+  bool CanBeCall;
+};
+MATCHER_P(named, name, "") { return arg.Name == name; }
+MATCHER_P(isStatic, value, "") { return arg.IsStatic == value; }
+MATCHER_P(canBeCall, value, "") { return arg.CanBeCall == value; }
+
+class SaveCompletedFunctions : public CodeCompleteConsumer {
+public:
+  SaveCompletedFunctions(std::vector )
+  : CodeCompleteConsumer(/*CodeCompleteOpts=*/{}),
+CompletedFuncDecls(CompletedFuncDecls),
+CCTUInfo(std::make_shared()) {}
+
+  void ProcessCodeCompleteResults(Sema , CodeCompletionContext Context,
+  CodeCompletionResult *Results,
+  unsigned NumResults) override {
+for (unsigned I = 0; I < NumResults; ++I) {
+  auto R = Results[I];
+  if (R.Kind == CodeCompletionResult::RK_Declaration) {
+if (const auto *FD = llvm::dyn_cast(R.getDeclaration())) {
+  CompletedFunctionDecl D;
+  D.Name = FD->getNameAsString();
+  D.CanBeCall = R.FunctionCanBeCall;
+  D.IsStatic = FD->isStatic();
+  CompletedFuncDecls.emplace_back(std::move(D));
+}
+  }
+}
+  }
+
+private:
+  CodeCompletionAllocator () override {
+return CCTUInfo.getAllocator();
+  }
+
+  CodeCompletionTUInfo () override { return CCTUInfo; }
+
+  std::vector 
+
+  CodeCompletionTUInfo CCTUInfo;
+};
+
 class VisitedContextFinder : public CodeCompleteConsumer {
 public:
   VisitedContextFinder(CompletionContext )
@@ -74,19 +121,19 @@
 
 class CodeCompleteAction : public SyntaxOnlyAction {
 public:
-  CodeCompleteAction(ParsedSourceLocation P, CompletionContext )
-  : CompletePosition(std::move(P)), ResultCtx(ResultCtx) {}
+  CodeCompleteAction(ParsedSourceLocation P, CodeCompleteConsumer *Consumer)
+  : CompletePosition(std::move(P)), Consumer(Consumer) {}
 
   bool BeginInvocation(CompilerInstance ) override {
 CI.getFrontendOpts().CodeCompletionAt = CompletePosition;
-CI.setCodeCompletionConsumer(new VisitedContextFinder(ResultCtx));
+CI.setCodeCompletionConsumer(Consumer);
 return true;
   }
 
 private:
   // 1-based code complete position ;
   ParsedSourceLocation CompletePosition;
-  CompletionContext 
+  CodeCompleteConsumer *Consumer;
 };
 
 ParsedSourceLocation offsetToPosition(llvm::StringRef Code, size_t Offset) {
@@ -103,7 +150,7 @@
   CompletionContext ResultCtx;
   clang::tooling::runToolOnCodeWithArgs(
   std::make_unique(offsetToPosition(Code, Offset),
-   ResultCtx),
+   new VisitedContextFinder(ResultCtx)),
   Code, {"-std=c++11"}, TestCCName);
   return ResultCtx;
 }
@@ -129,6 +176,69 @@
   return Types;
 }
 
+std::vector
+CollectCompletedFunctions(StringRef Code, std::size_t Point) {
+  std::vector Result;
+  clang::tooling::runToolOnCodeWithArgs(
+  std::make_unique(offsetToPosition(Code, Point),
+   new SaveCompletedFunctions(Result)),
+  Code, {"-std=c++11"}, TestCCName);
+  return Result;
+}
+
+TEST(SemaCodeCompleteTest, FunctionCanBeCall) {
+  llvm::Annotations Code(R"cpp(
+struct Foo {
+  static int staticMethod();
+  int method() const;
+  Foo() {
+this->$canBeCall^
+$canBeCall^
+Foo::$canBeCall^
+  }
+};
+
+struct Derived : Foo {
+  Derived() {
+Foo::$canBeCall^
+  }
+};
+
+struct OtherClass {
+  OtherClass() {
+Foo f;
+f.$canBeCall^
+::$cannotBeCall^
+  }
+};
+
+int main() {
+  Foo f;
+  f.$canBeCall^
+  ::$cannotBeCall^
+}
+)cpp");
+
+  for (const auto  : Code.points("canBeCall")) {
+auto Results = CollectCompletedFunctions(Code.code(), P);
+EXPECT_THAT(Results, Contains(AllOf(named("method"), isStatic(false),
+canBeCall(true;
+  }
+
+  for (const auto  : Code.points("cannotBeCall")) {
+auto Results = 

[PATCH] D137040: [clangd] Add heuristic for dropping snippet when completing member function pointer

[Nathan Ridge via Phabricator via cfe-commits]

nridge added a comment.

Thanks!

In D137040#3918118 , @tom-anders 
wrote:

> now we kinda have the same test case duplicated in sema and clangd tests - I 
> guess for clangd we now actually only have to test that the SnippetSuffix is 
> cleared when FunctionCanBeCall is true, but I don't see an easy way to 
> somehow inject fake Sema results into CodeComplete.cpp

I don't mind the duplication too much; I like that the clangd test is an "end 
to end" test directly expressing the user-visible behaviour that "in these 
situations we should get these completions".




Comment at: clang/unittests/Sema/CodeCompleteTest.cpp:123
+  CodeCompleteAction(ParsedSourceLocation P, CompletionContext ,
+ CodeCompleteConsumer *Consumer = nullptr)
+  : CompletePosition(std::move(P)), ResultCtx(ResultCtx),

Since the constructor has only one other caller (`runCompletion()`), I'd rather 
remove the default arg and put the `new VisitedContextFinder(...)` in that 
caller.

As a bonus, we would no longer need to pass the `CompletionContext` to the 
`CodeCompleteAction` constructor, and don't need to use a "dummy context" in 
`CollectCompletedFunctions()`.



Comment at: clang/unittests/Sema/CodeCompleteTest.cpp:231
+Contains(AllOf(
+Field("Name", ::Name, "method"),
+Field("IsStatic", ::IsStatic, false),

Consider using [this 
style](https://searchfox.org/llvm/rev/3182ea4a8fcb163c6e5cb01f474f84f30d101dd9/clang-tools-extra/clangd/unittests/TypeHierarchyTests.cpp#35-37)
 of matcher ([example 
use](https://searchfox.org/llvm/rev/3182ea4a8fcb163c6e5cb01f474f84f30d101dd9/clang-tools-extra/clangd/unittests/TypeHierarchyTests.cpp#378)).


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D137040/new/

https://reviews.llvm.org/D137040

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D137040: [clangd] Add heuristic for dropping snippet when completing member function pointer

[Tom Praschan via Phabricator via cfe-commits]

tom-anders updated this revision to Diff 474818.
tom-anders added a comment.

Fix test


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D137040/new/

https://reviews.llvm.org/D137040

Files:
  clang-tools-extra/clangd/CodeComplete.cpp
  clang-tools-extra/clangd/unittests/CodeCompleteTests.cpp
  clang/include/clang/Sema/CodeCompleteConsumer.h
  clang/lib/Sema/SemaCodeComplete.cpp
  clang/unittests/Sema/CodeCompleteTest.cpp

Index: clang/unittests/Sema/CodeCompleteTest.cpp
===
--- clang/unittests/Sema/CodeCompleteTest.cpp
+++ clang/unittests/Sema/CodeCompleteTest.cpp
@@ -23,7 +23,10 @@
 
 using namespace clang;
 using namespace clang::tooling;
+using ::testing::AllOf;
+using ::testing::Contains;
 using ::testing::Each;
+using ::testing::Field;
 using ::testing::UnorderedElementsAre;
 
 const char TestCCName[] = "test.cc";
@@ -36,6 +39,48 @@
   std::string PtrDiffType;
 };
 
+struct CompletedFunctionDecl {
+  std::string Name;
+  bool IsStatic;
+  bool CanBeCall;
+};
+
+class SaveCompletedFunctions : public CodeCompleteConsumer {
+public:
+  SaveCompletedFunctions(std::vector )
+  : CodeCompleteConsumer(/*CodeCompleteOpts=*/{}),
+CompletedFuncDecls(CompletedFuncDecls),
+CCTUInfo(std::make_shared()) {}
+
+  void ProcessCodeCompleteResults(Sema , CodeCompletionContext Context,
+  CodeCompletionResult *Results,
+  unsigned NumResults) override {
+for (unsigned I = 0; I < NumResults; ++I) {
+  auto R = Results[I];
+  if (R.Kind == CodeCompletionResult::RK_Declaration) {
+if (const auto *FD = llvm::dyn_cast(R.getDeclaration())) {
+  CompletedFunctionDecl D;
+  D.Name = FD->getNameAsString();
+  D.CanBeCall = R.FunctionCanBeCall;
+  D.IsStatic = FD->isStatic();
+  CompletedFuncDecls.emplace_back(std::move(D));
+}
+  }
+}
+  }
+
+private:
+  CodeCompletionAllocator () override {
+return CCTUInfo.getAllocator();
+  }
+
+  CodeCompletionTUInfo () override { return CCTUInfo; }
+
+  std::vector 
+
+  CodeCompletionTUInfo CCTUInfo;
+};
+
 class VisitedContextFinder : public CodeCompleteConsumer {
 public:
   VisitedContextFinder(CompletionContext )
@@ -74,12 +119,15 @@
 
 class CodeCompleteAction : public SyntaxOnlyAction {
 public:
-  CodeCompleteAction(ParsedSourceLocation P, CompletionContext )
-  : CompletePosition(std::move(P)), ResultCtx(ResultCtx) {}
+  CodeCompleteAction(ParsedSourceLocation P, CompletionContext ,
+ CodeCompleteConsumer *Consumer = nullptr)
+  : CompletePosition(std::move(P)), ResultCtx(ResultCtx),
+Consumer(Consumer) {}
 
   bool BeginInvocation(CompilerInstance ) override {
 CI.getFrontendOpts().CodeCompletionAt = CompletePosition;
-CI.setCodeCompletionConsumer(new VisitedContextFinder(ResultCtx));
+CI.setCodeCompletionConsumer(
+Consumer ? Consumer : new VisitedContextFinder(ResultCtx));
 return true;
   }
 
@@ -87,6 +135,7 @@
   // 1-based code complete position ;
   ParsedSourceLocation CompletePosition;
   CompletionContext 
+  CodeCompleteConsumer *Consumer;
 };
 
 ParsedSourceLocation offsetToPosition(llvm::StringRef Code, size_t Offset) {
@@ -129,6 +178,83 @@
   return Types;
 }
 
+std::vector
+CollectCompletedFunctions(StringRef Code, std::size_t Point) {
+  CompletionContext DummyContext;
+  std::vector Result;
+  clang::tooling::runToolOnCodeWithArgs(
+  std::make_unique(offsetToPosition(Code, Point),
+   DummyContext,
+   new SaveCompletedFunctions(Result)),
+  Code, {"-std=c++11"}, TestCCName);
+  return Result;
+}
+
+TEST(SemaCodeCompleteTest, FunctionCanBeCall) {
+  llvm::Annotations Code(R"cpp(
+struct Foo {
+  static int staticMethod();
+  int method() const;
+  Foo() {
+this->$canBeCall^
+$canBeCall^
+Foo::$canBeCall^
+  }
+};
+
+struct Derived : Foo {
+  Derived() {
+Foo::$canBeCall^
+  }
+};
+
+struct OtherClass {
+  OtherClass() {
+Foo f;
+f.$canBeCall^
+::$cannotBeCall^
+  }
+};
+
+int main() {
+  Foo f;
+  f.$canBeCall^
+  ::$cannotBeCall^
+}
+)cpp");
+
+  for (const auto  : Code.points("canBeCall")) {
+auto Results = CollectCompletedFunctions(Code.code(), P);
+EXPECT_THAT(
+Results,
+Contains(AllOf(
+Field("Name", ::Name, "method"),
+Field("IsStatic", ::IsStatic, false),
+Field("CanBeCall", ::CanBeCall, true;
+  }
+
+  for (const auto  : Code.points("cannotBeCall")) {
+auto Results = CollectCompletedFunctions(Code.code(), P);
+EXPECT_THAT(
+Results,
+Contains(AllOf(
+Field("Name", ::Name, "method"),
+

[PATCH] D137040: [clangd] Add heuristic for dropping snippet when completing member function pointer

[Tom Praschan via Phabricator via cfe-commits]

tom-anders added a comment.

Hmm I added the test for the flag to Sema, but now we kinda have the same test 
case duplicated in sema and clangd tests - I guess for clangd we now actually 
only have to test that the SnippetSuffix is cleared when FunctionCanBeCall is 
true, but I don't see an easy way to somehow inject fake Sema results into 
CodeComplete.cpp


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D137040/new/

https://reviews.llvm.org/D137040

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D137040: [clangd] Add heuristic for dropping snippet when completing member function pointer

[Tom Praschan via Phabricator via cfe-commits]

tom-anders updated this revision to Diff 474358.
tom-anders added a comment.

Add test to sema


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D137040/new/

https://reviews.llvm.org/D137040

Files:
  clang-tools-extra/clangd/CodeComplete.cpp
  clang-tools-extra/clangd/unittests/CodeCompleteTests.cpp
  clang/include/clang/Sema/CodeCompleteConsumer.h
  clang/lib/Sema/SemaCodeComplete.cpp
  clang/unittests/Sema/CodeCompleteTest.cpp

Index: clang/unittests/Sema/CodeCompleteTest.cpp
===
--- clang/unittests/Sema/CodeCompleteTest.cpp
+++ clang/unittests/Sema/CodeCompleteTest.cpp
@@ -23,7 +23,10 @@
 
 using namespace clang;
 using namespace clang::tooling;
+using ::testing::AllOf;
+using ::testing::Contains;
 using ::testing::Each;
+using ::testing::Field;
 using ::testing::UnorderedElementsAre;
 
 const char TestCCName[] = "test.cc";
@@ -36,6 +39,48 @@
   std::string PtrDiffType;
 };
 
+struct CompletedFunctionDecl {
+  std::string Name;
+  bool IsStatic;
+  bool CanBeCall;
+};
+
+class SaveCompletedFunctions : public CodeCompleteConsumer {
+public:
+  SaveCompletedFunctions(std::vector )
+  : CodeCompleteConsumer(/*CodeCompleteOpts=*/{}),
+CompletedFuncDecls(CompletedFuncDecls),
+CCTUInfo(std::make_shared()) {}
+
+  void ProcessCodeCompleteResults(Sema , CodeCompletionContext Context,
+  CodeCompletionResult *Results,
+  unsigned NumResults) override {
+for (unsigned I = 0; I < NumResults; ++I) {
+  auto R = Results[I];
+  if (R.Kind == CodeCompletionResult::RK_Declaration) {
+if (const auto *FD = llvm::dyn_cast(R.getDeclaration())) {
+  CompletedFunctionDecl D;
+  D.Name = FD->getNameAsString();
+  D.CanBeCall = R.FunctionCanBeCall;
+  D.IsStatic = FD->isStatic();
+  CompletedFuncDecls.emplace_back(std::move(D));
+}
+  }
+}
+  }
+
+private:
+  CodeCompletionAllocator () override {
+return CCTUInfo.getAllocator();
+  }
+
+  CodeCompletionTUInfo () override { return CCTUInfo; }
+
+  std::vector 
+
+  CodeCompletionTUInfo CCTUInfo;
+};
+
 class VisitedContextFinder : public CodeCompleteConsumer {
 public:
   VisitedContextFinder(CompletionContext )
@@ -74,12 +119,15 @@
 
 class CodeCompleteAction : public SyntaxOnlyAction {
 public:
-  CodeCompleteAction(ParsedSourceLocation P, CompletionContext )
-  : CompletePosition(std::move(P)), ResultCtx(ResultCtx) {}
+  CodeCompleteAction(ParsedSourceLocation P, CompletionContext ,
+ CodeCompleteConsumer *Consumer = nullptr)
+  : CompletePosition(std::move(P)), ResultCtx(ResultCtx),
+Consumer(Consumer) {}
 
   bool BeginInvocation(CompilerInstance ) override {
 CI.getFrontendOpts().CodeCompletionAt = CompletePosition;
-CI.setCodeCompletionConsumer(new VisitedContextFinder(ResultCtx));
+CI.setCodeCompletionConsumer(
+Consumer ? Consumer : new VisitedContextFinder(ResultCtx));
 return true;
   }
 
@@ -87,6 +135,7 @@
   // 1-based code complete position ;
   ParsedSourceLocation CompletePosition;
   CompletionContext 
+  CodeCompleteConsumer *Consumer;
 };
 
 ParsedSourceLocation offsetToPosition(llvm::StringRef Code, size_t Offset) {
@@ -129,6 +178,83 @@
   return Types;
 }
 
+std::vector
+CollectCompletedFunctions(StringRef Code, std::size_t Point) {
+  CompletionContext DummyContext;
+  std::vector Result;
+  clang::tooling::runToolOnCodeWithArgs(
+  std::make_unique(offsetToPosition(Code, Point),
+   DummyContext,
+   new SaveCompletedFunctions(Result)),
+  Code, {"-std=c++11"}, TestCCName);
+  return Result;
+}
+
+TEST(SemaCodeCompleteTest, FunctionCanBeCall) {
+  llvm::Annotations Code(R"cpp(
+struct Foo {
+  static int staticMethod();
+  int method() const;
+  Foo() {
+this->$canBeCall^
+$canBeCall^
+Foo::$canBeCall^
+  }
+};
+
+struct Derived : Foo {
+  Derived() {
+Foo::$canBeCall^
+  }
+};
+
+struct OtherClass {
+  OtherClass() {
+Foo f;
+f.$canBeCall^
+::$cannotBeCall^
+  }
+};
+
+int main() {
+  Foo f;
+  f.$canBeCall^
+  ::$cannotBeCall^
+}
+)cpp");
+
+  for (const auto  : Code.points("canBeCall")) {
+auto Results = CollectCompletedFunctions(Code.code(), P);
+EXPECT_THAT(
+Results,
+Contains(AllOf(
+Field("Name", ::Name, "method"),
+Field("IsStatic", ::IsStatic, false),
+Field("CanBeCall", ::CanBeCall, true;
+  }
+
+  for (const auto  : Code.points("keepSnippet")) {
+auto Results = CollectCompletedFunctions(Code.code(), P);
+EXPECT_THAT(
+Results,
+Contains(AllOf(
+Field("Name", ::Name, "method"),
+

[PATCH] D137040: [clangd] Add heuristic for dropping snippet when completing member function pointer

[Nathan Ridge via Phabricator via cfe-commits]

nridge added a comment.

In D137040#3909906 , @tom-anders 
wrote:

> We now probably also need a unit test for Sema that tests the new flag, right?

Good point. My understanding is that most test coverage of libSema's code 
completion takes the form of lit tests 
 
which are suitable for checking the completion proposals themselves but not 
flags like this. However, we do have one unittest file 
 
which could be adapted for testing the flag. It may require some light 
refactoring to make this work (e.g. I don't think we can just stuff the 
`CodeCompletionResult`s themselves into the returned `CompletionContext` 
because they point to `Decl`s and such whose lifetime is tied to the AST which 
is destroyed by the time `runCompletion()` returns).




Comment at: clang-tools-extra/clangd/CodeComplete.cpp:414
, IsPattern);
+  if (!C.SemaResult->FunctionCanBeCall)
+S.SnippetSuffix.clear();

tom-anders wrote:
> Here, the SnippetSuffix is still created and then immediately cleared again. 
> But the logic inside getSignature is quite complex and it's used in multiple 
> places, so I didn't really want to touch that function for now.
Fair enough, we can optimize this later if it turns out to be necessary


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D137040/new/

https://reviews.llvm.org/D137040

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D137040: [clangd] Add heuristic for dropping snippet when completing member function pointer

[Tom Praschan via Phabricator via cfe-commits]

tom-anders added a comment.

The test in clangd/unittests/CodeCompleteTests.cpp still passes. We now 
probably also need a unit test for Sema that tests the new flag, right?




Comment at: clang-tools-extra/clangd/CodeComplete.cpp:414
, IsPattern);
+  if (!C.SemaResult->FunctionCanBeCall)
+S.SnippetSuffix.clear();

Here, the SnippetSuffix is still created and then immediately cleared again. 
But the logic inside getSignature is quite complex and it's used in multiple 
places, so I didn't really want to touch that function for now.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D137040/new/

https://reviews.llvm.org/D137040

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D137040: [clangd] Add heuristic for dropping snippet when completing member function pointer

[Tom Praschan via Phabricator via cfe-commits]

tom-anders updated this revision to Diff 473408.
tom-anders added a comment.
Herald added a project: clang.

Move logic to SemaCodeComplete.cpp


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D137040/new/

https://reviews.llvm.org/D137040

Files:
  clang-tools-extra/clangd/CodeComplete.cpp
  clang-tools-extra/clangd/unittests/CodeCompleteTests.cpp
  clang/include/clang/Sema/CodeCompleteConsumer.h
  clang/lib/Sema/SemaCodeComplete.cpp

Index: clang/lib/Sema/SemaCodeComplete.cpp
===
--- clang/lib/Sema/SemaCodeComplete.cpp
+++ clang/lib/Sema/SemaCodeComplete.cpp
@@ -1379,6 +1379,33 @@
 OverloadSet.Add(Method, Results.size());
   }
 
+  // When completing a non-static member function (and not via
+  // dot/arrow member access) and we're not inside that class' scope,
+  // it can't be a call.
+  if (CompletionContext.getKind() == clang::CodeCompletionContext::CCC_Symbol) {
+const auto *Method = dyn_cast(R.getDeclaration());
+if (Method && !Method->isStatic()) {
+  // Find the class scope that we're currently in.
+  // We could e.g. be inside a lambda, so walk up the DeclContext until we
+  // find a CXXMethodDecl.
+  const auto *CurrentClassScope = [&]() -> const CXXRecordDecl * {
+for (DeclContext *Ctx = SemaRef.CurContext; Ctx;
+ Ctx = Ctx->getParent()) {
+  const auto *CtxMethod = llvm::dyn_cast(Ctx);
+  if (CtxMethod && !CtxMethod->getParent()->isLambda()) {
+return CtxMethod->getParent();
+  }
+}
+return nullptr;
+  }();
+
+  R.FunctionCanBeCall =
+  CurrentClassScope &&
+  (CurrentClassScope == Method->getParent() ||
+   CurrentClassScope->isDerivedFrom(Method->getParent()));
+}
+  }
+
   // Insert this result into the set of results.
   Results.push_back(R);
 
Index: clang/include/clang/Sema/CodeCompleteConsumer.h
===
--- clang/include/clang/Sema/CodeCompleteConsumer.h
+++ clang/include/clang/Sema/CodeCompleteConsumer.h
@@ -850,6 +850,12 @@
   /// rather than a use of that entity.
   bool DeclaringEntity : 1;
 
+  /// When completing a function, whether it can be a call. This will usually be
+  /// true, but we have some heuristics, e.g. when a pointer to a non-static
+  /// member function is completed outside of that class' scope, it can never
+  /// be a call.
+  bool FunctionCanBeCall : 1;
+
   /// If the result should have a nested-name-specifier, this is it.
   /// When \c QualifierIsInformative, the nested-name-specifier is
   /// informative rather than required.
@@ -876,7 +882,7 @@
 FixIts(std::move(FixIts)), Hidden(false), InBaseClass(false),
 QualifierIsInformative(QualifierIsInformative),
 StartsNestedNameSpecifier(false), AllParametersAreInformative(false),
-DeclaringEntity(false), Qualifier(Qualifier) {
+DeclaringEntity(false), FunctionCanBeCall(true), Qualifier(Qualifier) {
 // FIXME: Add assert to check FixIts range requirements.
 computeCursorKindAndAvailability(Accessible);
   }
@@ -886,7 +892,8 @@
   : Keyword(Keyword), Priority(Priority), Kind(RK_Keyword),
 CursorKind(CXCursor_NotImplemented), Hidden(false), InBaseClass(false),
 QualifierIsInformative(false), StartsNestedNameSpecifier(false),
-AllParametersAreInformative(false), DeclaringEntity(false) {}
+AllParametersAreInformative(false), DeclaringEntity(false),
+FunctionCanBeCall(true) {}
 
   /// Build a result that refers to a macro.
   CodeCompletionResult(const IdentifierInfo *Macro,
@@ -896,7 +903,7 @@
 CursorKind(CXCursor_MacroDefinition), Hidden(false), InBaseClass(false),
 QualifierIsInformative(false), StartsNestedNameSpecifier(false),
 AllParametersAreInformative(false), DeclaringEntity(false),
-MacroDefInfo(MI) {}
+FunctionCanBeCall(true), MacroDefInfo(MI) {}
 
   /// Build a result that refers to a pattern.
   CodeCompletionResult(
@@ -908,7 +915,7 @@
 CursorKind(CursorKind), Availability(Availability), Hidden(false),
 InBaseClass(false), QualifierIsInformative(false),
 StartsNestedNameSpecifier(false), AllParametersAreInformative(false),
-DeclaringEntity(false) {}
+DeclaringEntity(false), FunctionCanBeCall(true) {}
 
   /// Build a result that refers to a pattern with an associated
   /// declaration.
@@ -917,7 +924,7 @@
   : Declaration(D), Pattern(Pattern), Priority(Priority), Kind(RK_Pattern),
 Hidden(false), InBaseClass(false), QualifierIsInformative(false),
 StartsNestedNameSpecifier(false), AllParametersAreInformative(false),
-DeclaringEntity(false) {
+DeclaringEntity(false), FunctionCanBeCall(true) {
 computeCursorKindAndAvailability();
   }
 
Index: 

[PATCH] D137040: [clangd] Add heuristic for dropping snippet when completing member function pointer

[Tom Praschan via Phabricator via cfe-commits]

tom-anders added a comment.

In D137040#3907497 , @nridge wrote:

> Thanks for the patch!
>
> The test looks good to me.
>
> As for the fix, I wonder if it would make sense to implement it in the Sema 
> layer rather than in clangd. For example, we could give 
> `clang::CodeCompletionResult` a new flag 
> 
>  called something like `CanBeCall`, which, for a completion result that 
> refers to a function, is set to true (by the code that creates the 
> CodeCompletionResult, in SemaCodeComplete.cpp) if the use of the function may 
> be a call, and false if it cannot.
>
> Then clangd can check this flag, and clear the snippet suffix (or even 
> better, not build it in the first place) if the value is false.
>
> I think this approach could have a couple of advantages:
>
> 1. By placing the logic into libSema, other consumers of the Sema completion 
> layer, besides clangd, could benefit from it as well. (A couple of other 
> consumers that I'm aware of are libclang 
>  and cling 
> .)
> 2. Because the logic now resides inside `Sema` itself, if we make 
> enhancements to the heuristics for computing `CanBeCall` that require 
> additional state (besides `CurContext`), we can just use them in place rather 
> than having to propagate them into places like clangd's 
> `CodeCompletionBuilder`.
>
> What do you think?

That indeed sounds like a better solution. I haven't looked much into 
SemaCodeComplete.cpp up to now, but I'll take a look and try to implement the 
heuristic over there.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D137040/new/

https://reviews.llvm.org/D137040

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D137040: [clangd] Add heuristic for dropping snippet when completing member function pointer

[Nathan Ridge via Phabricator via cfe-commits]

nridge added a comment.

Thanks for the patch!

The test looks good to me.

As for the fix, I wonder if it would make sense to implement it in the Sema 
layer rather than in clangd. For example, we could give 
`clang::CodeCompletionResult` a new flag 
 called something like 
`CanBeCall`, which, for a completion result that refers to a function, is set 
to true (by the code that creates the CodeCompletionResult, in 
SemaCodeComplete.cpp) if the use of the function may be a call, and false if it 
cannot.

Then clangd can check this flag, and clear the snippet suffix (or even better, 
not build it in the first place) if the value is false.

I think this approach could have a couple of advantages:

1. By placing the logic into libSema, other consumers of the Sema completion 
layer, besides clangd, could benefit from it as well. (A couple of other 
consumers that I'm aware of are libclang 
 and cling 
.)
2. Because the logic now resides inside `Sema` itself, if we make enhancements 
to the heuristics for computing `CanBeCall` that require additional state 
(besides `CurContext`), we can just use them in place rather than having to 
propagate them into places like clangd's `CodeCompletionBuilder`.

What do you think?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D137040/new/

https://reviews.llvm.org/D137040

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D137040: [clangd] Add heuristic for dropping snippet when completing member function pointer

[Tom Praschan via Phabricator via cfe-commits]

tom-anders created this revision.
Herald added subscribers: kadircet, arphaman.
Herald added a project: All.
tom-anders requested review of this revision.
Herald added subscribers: cfe-commits, MaskRay, ilya-biryukov.
Herald added a project: clang-tools-extra.

This implements the 1st heuristic mentioned in 
https://github.com/clangd/clangd/issues/968#issuecomment-1002242704:

When completing a function that names a non-static member of a class, and we 
are not inside that class's scope, assume the reference will not be a call (and 
thus don't add the snippetSuffix)


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D137040

Files:
  clang-tools-extra/clangd/CodeComplete.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
@@ -500,6 +500,63 @@
  snippetSuffix("(${1:int i}, ${2:const float f})")));
 }
 
+TEST(CompletionTest, HeuristicsForMemberFunctionCompletion) {
+  clangd::CodeCompleteOptions Opts;
+  Opts.EnableSnippets = true;
+
+  Annotations Code(R"cpp(
+  struct Foo {
+static int staticMethod();
+int method() const;
+Foo() {
+  this->$keepSnippet^
+  $keepSnippet^
+  Foo::$keepSnippet^
+}
+  };
+
+  struct Derived : Foo {
+Derived() {
+  Foo::$keepSnippet^
+}
+  };
+
+  struct OtherClass {
+OtherClass() {
+  Foo f;
+  f.$keepSnippet^
+  ::$noSnippet^
+}
+  };
+
+  int main() {
+Foo f;
+f.$keepSnippet^
+::$noSnippet^
+  }
+  )cpp");
+  auto TU = TestTU::withCode(Code.code());
+
+  for (const auto  : Code.points("noSnippet")) {
+auto Results = completions(TU, P, /*IndexSymbols*/ {}, Opts);
+EXPECT_THAT(Results.Completions,
+Contains(AllOf(named("method"), snippetSuffix("";
+  }
+
+  for (const auto  : Code.points("keepSnippet")) {
+auto Results = completions(TU, P, /*IndexSymbols*/ {}, Opts);
+EXPECT_THAT(Results.Completions,
+Contains(AllOf(named("method"), snippetSuffix("()";
+  }
+
+  // static method will always keep the snippet
+  for (const auto  : Code.points()) {
+auto Results = completions(TU, P, /*IndexSymbols*/ {}, Opts);
+EXPECT_THAT(Results.Completions,
+Contains(AllOf(named("staticMethod"), snippetSuffix("()";
+  }
+}
+
 TEST(CompletionTest, NoSnippetsInUsings) {
   clangd::CodeCompleteOptions Opts;
   Opts.EnableSnippets = true;
Index: clang-tools-extra/clangd/CodeComplete.cpp
===
--- clang-tools-extra/clangd/CodeComplete.cpp
+++ clang-tools-extra/clangd/CodeComplete.cpp
@@ -291,7 +291,8 @@
 // computed from the first candidate, in the constructor.
 // Others vary per candidate, so add() must be called for remaining candidates.
 struct CodeCompletionBuilder {
-  CodeCompletionBuilder(ASTContext *ASTCtx, const CompletionCandidate ,
+  CodeCompletionBuilder(ASTContext *ASTCtx, DeclContext *SemaDeclCtx,
+const CompletionCandidate ,
 CodeCompletionString *SemaCCS,
 llvm::ArrayRef QueryScopes,
 const IncludeInserter ,
@@ -299,13 +300,15 @@
 CodeCompletionContext::Kind ContextKind,
 const CodeCompleteOptions ,
 bool IsUsingDeclaration, tok::TokenKind NextTokenKind)
-  : ASTCtx(ASTCtx),
+  : ASTCtx(ASTCtx), SemaDeclCtx(SemaDeclCtx),
 EnableFunctionArgSnippets(Opts.EnableFunctionArgSnippets),
-IsUsingDeclaration(IsUsingDeclaration), NextTokenKind(NextTokenKind) {
+ContextKind(ContextKind), IsUsingDeclaration(IsUsingDeclaration),
+NextTokenKind(NextTokenKind) {
 Completion.Deprecated = true; // cleared by any non-deprecated overload.
 add(C, SemaCCS);
 if (C.SemaResult) {
   assert(ASTCtx);
+  assert(SemaDeclCtx);
   Completion.Origin |= SymbolOrigin::AST;
   Completion.Name = std::string(llvm::StringRef(SemaCCS->getTypedText()));
   Completion.FilterText = SemaCCS->getAllTypedText();
@@ -412,6 +415,8 @@
   getSignature(*SemaCCS, , ,
, IsPattern);
   S.ReturnType = getReturnType(*SemaCCS);
+  maybeClearSnippetSuffixForMethodFunctionPointer(*C.SemaResult,
+  S.SnippetSuffix);
 } else if (C.IndexResult) {
   S.Signature = std::string(C.IndexResult->Signature);
   S.SnippetSuffix = std::string(C.IndexResult->CompletionSnippetSuffix);
@@ -570,11 +575,57 @@
 return "(…)";
   }
 
+  // Clear snippet when completing a non-static member function (and