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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
