nridge added a comment.

In D137040#3909906 <https://reviews.llvm.org/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 
<https://searchfox.org/llvm/source/clang/test/CodeCompletion/using-enum.cpp> 
which are suitable for checking the completion proposals themselves but not 
flags like this. However, we do have one unittest file 
<https://searchfox.org/llvm/source/clang/unittests/Sema/CodeCompleteTest.cpp> 
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
                    &Completion.RequiredQualifier, 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

Reply via email to