nridge added a comment.
Thanks for the patch!
In the future we may want to add other cursor kinds for which
`ShouldPatchPlaceholder0` should be false, but I think this is a good start
which addresses the case from the bug.
================
Comment at: clang-tools-extra/clangd/CodeComplete.cpp:441
getSignature(*SemaCCS, &S.Signature, &S.SnippetSuffix,
- &Completion.RequiredQualifier, IsPattern);
+ &Completion.RequiredQualifier, IsPattern, C.SemaResult);
if (!C.SemaResult->FunctionCanBeCall)
----------------
Since we are now passing in the entire `C.SemaResult`, can we remove the
`CompletingPattern` parameter, and compute that boolean inside the function
instead?
================
Comment at: clang-tools-extra/clangd/CodeCompletionStrings.cpp:104
unsigned CursorSnippetArg = std::numeric_limits<unsigned>::max();
if (CompletingPattern) {
// In patterns, it's best to place the cursor at the last placeholder, to
----------------
Can we move this check after the declaration of `ShouldPatchPlaceholder0`, and
change the condition to `if (ShouldPatchPlaceholder0)`?
(And at the usage site, swap the operands to `if (ShouldPatchPlaceholder0 &&
SnippetArg == CursorSnippetArg)`?)
This would avoid doing the count_if when not necessary.
================
Comment at: clang-tools-extra/clangd/CodeCompletionStrings.cpp:130
+ return false;
+ return true;
+ }();
----------------
zyounan wrote:
> I was cringed that if we should refine the logic based on `CursorKind`: It is
> from libclang; The meaning is sometimes kind of opaque (to me, I don't know
> it very clearly TBH) like `CXCursor_NotImplemented`...
It does seem like a layering violation that a libSema interface
(CodeCompletionResult) uses a libclang type (CXCursorKind), but that's a
pre-existing issue.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D145319/new/
https://reviews.llvm.org/D145319
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
