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

[Nathan Ridge via Phabricator via cfe-commits]

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