[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 <https://reviews.llvm.org/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 &ResultCtx,
+                     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", &CompletedFunctionDecl::Name, "method"),
+            Field("IsStatic", &CompletedFunctionDecl::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