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

[Kirill Bobyrev via Phabricator via cfe-commits]

kbobyrev added inline comments.


================
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,
----------------
sammccall wrote:
> need to decide what to do when it returns none, or add an explicit assert with a message ("code completing in macro?")
Yeah, forgot about that -_- I think just doing nothing is OK here?


================
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
----------------
sammccall wrote:
> 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?`
I'd like to do that next patch (this patch is actually a simplified version of what I *wanted* to do, but I figured there are too many corner cases and complications. I would like to get to that very soon but I'm just afraid I'll be distracted or something. I should probably just write this down for myself, that would be fine.


================
Comment at: clang-tools-extra/clangd/CodeComplete.cpp:1712
+            SemaCCS, QueryScopes, *Inserter, FileName, CCContextKind, Opts,
+            /*GenerateSnippets=*/!IsUsingDeclaration && !HasParenthesisAfter);
       else
----------------
sammccall wrote:
> 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.
Yeah templates are hard :( The problem here is that this is a heuristic and I was considering suppressing pieces of the CC snippets (e.g. the templated parts) upon having `<` token ahead but this can also be "less than" sign and the heuristics are probably going to be harder for this. Maybe some semantic analysis would do, but I'm not sure about all the details right now.

I've added a test which shows how the template snippet will be omitted, is that OK with you?


================
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(""))));
----------------
sammccall wrote:
> can we have fo^o(42) as a case as well?
We can, but it wouldn't be handled "as expected" :) The problem would be that the next token is an identifier (`o`) and hence this wouldn't work. I'd like to fix that too (see if the next token is an identifier and a suffix of inserted text), but I think this should be a separate patch.

Do you want me to add this case with a FIXME?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D81380