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



_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Reply via email to