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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
