sammccall accepted this revision.
sammccall marked 2 inline comments as done.
sammccall added inline comments.
This revision is now accepted and ready to land.
================
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,
----------------
kbobyrev wrote:
> 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?
Yup EOF LGTM.
================
Comment at: clang-tools-extra/clangd/CodeComplete.cpp:1712
+ SemaCCS, QueryScopes, *Inserter, FileName, CCContextKind, Opts,
+ /*GenerateSnippets=*/!IsUsingDeclaration && !HasParenthesisAfter);
else
----------------
kbobyrev wrote:
> 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?
Yes, adding a test showing we don't handle this edge case is fine.
================
Comment at: clang-tools-extra/clangd/CodeComplete.cpp:1715
+ CCContextKind, Opts, !IsUsingDeclaration,
+ NextTokenKind);
else
----------------
now you've got the "should we generate snippets" logic split a different way
:-) No-snippets-for-using-decl is expressed here, but no-args-if-parens-exist
is inside the builder.
I think it's slightly neater (less plumbing) to compute it here - it's safe to
do so based on the current item because we won't produce a bundle containing
functions together with other things.
Also fine to pass in IsUsingDeclaration, but it should be to a parameter called
"IsUsingDeclaration" rather than one called GenerateSnippets, as it's only part
of the equation.
================
Comment at: clang-tools-extra/clangd/unittests/CodeCompleteTests.cpp:2921
+ Contains(AllOf(Labeled("Container<typename T>(int Size)"),
+ SnippetSuffix(""),
+ Kind(CompletionItemKind::Constructor))));
----------------
Add a fixme: should be "<${1:typename T}>"
================
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(""))));
----------------
kbobyrev wrote:
> 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?
Yes, no need to handle it, but let's add a test and mark it as bad.
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
