[PATCH] D145319: [clangd] Refine logic on $0 in completion snippets

[Younan Zhang via Phabricator via cfe-commits]

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