zyounan added inline comments.
================
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)
----------------
nridge wrote:
> Since we are now passing in the entire `C.SemaResult`, can we remove the
> `CompletingPattern` parameter, and compute that boolean inside the function
> instead?
Revise the parameters of `getSignature` to circumvent CCR now -- Tests for
CodeCompletionStrings do not construct any CCR and I don't think it necessary
to add that logic.
================
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
----------------
nridge wrote:
> 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.
Nice catch! thanks!
================
Comment at: clang-tools-extra/clangd/CodeCompletionStrings.cpp:130
+ return false;
+ return true;
+ }();
----------------
nridge wrote:
> 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.
> It does seem like a layering violation that a libSema interface
> (CodeCompletionResult) uses a libclang type (CXCursorKind), but that's a
> pre-existing issue.
================
Comment at: clang-tools-extra/clangd/CodeCompletionStrings.cpp:130
+ return false;
+ return true;
+ }();
----------------
zyounan wrote:
> nridge wrote:
> > 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.
> > It does seem like a layering violation that a libSema interface
> > (CodeCompletionResult) uses a libclang type (CXCursorKind), but that's a
> > pre-existing issue.
>
>
Thanks for clarifying.
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
