[PATCH] D81380: [clangd] Don't produce snippets when completion location is followed by parenthesis

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

Thanks, this is nice!



================
Comment at: clang-tools-extra/clangd/CodeComplete.cpp:1228
+  // extra parenthesis.
+  bool HasParenthesisAfter = false;
   // Counters for logging.
----------------
I'd suggest rather storing the token kind as `NextToken`, and then deferring the actual "is it l_paren" check until toCodeCompletion.
That way the should-we-generate-snippets logic is more localized and IMO easier to read.


================
Comment at: clang-tools-extra/clangd/CodeComplete.cpp:1283
+          Recorder->CCSema->getSourceManager(), Recorder->CCSema->LangOpts);
+      HasParenthesisAfter = NextToken->getKind() == tok::l_paren;
       auto Style = getFormatStyleForFile(SemaCCInput.FileName,
----------------
need to decide what to do when it returns none, or add an explicit assert with a message ("code completing in macro?")


================
Comment at: clang-tools-extra/clangd/CodeComplete.cpp:1701
                           : nullptr;
+      // FIXME(kirillbobyrev): Instead of not generating any snippets when
+      // tok::l_paren is the next token after completion location, use more
----------------
This is a bit lengthy for describing an implementation strategy we're *not* using. Do you think we're very likely to do this, and this comment would save a lot of the work of finding out how?

I'd rather add a comment explaining what we *are* doing
e.g. `Suppress function argument snippets if args are already present, or not needed (using decl).`
And at most `// Should we consider sometimes replacing parens with the snippet instead?`


================
Comment at: clang-tools-extra/clangd/CodeComplete.cpp:1712
+            SemaCCS, QueryScopes, *Inserter, FileName, CCContextKind, Opts,
+            /*GenerateSnippets=*/!IsUsingDeclaration && !HasParenthesisAfter);
       else
----------------
how sure are we that paren-after is the right condition in all cases? Does "snippet" cover template snippets (std::vector<{$0}>)?

Don't need to handle this but it'd be nice to cover in tests.


================
Comment at: clang-tools-extra/clangd/unittests/CodeCompleteTests.cpp:2881
 
+TEST(CompletionTest, RemoveSnippetOnContext) {
+  clangd::CodeCompleteOptions Opts;
----------------
nit: I don't really follow this test name, but it seems to be explaining part of the implementation.

What about `FunctionArgsExist` which rather explains the scenario?


================
Comment at: clang-tools-extra/clangd/unittests/CodeCompleteTests.cpp:2893
+  EXPECT_THAT(
+      completions(Context + "int y = fo^(42)", {}, Opts).Completions,
+      UnorderedElementsAre(AllOf(Labeled("foo(int A)"), SnippetSuffix(""))));
----------------
can we have fo^o(42) as a case as well?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D81380