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

[Nathan Ridge via Phabricator via cfe-commits]

nridge added a comment.

Thanks for the update! I have a couple of more minor suggestions, hope you don't mind; I'm trying to make sure the next person to look at this will easily understand what the code is trying to do.



================
Comment at: clang-tools-extra/clangd/CodeCompletionStrings.cpp:112
+  // incorrect placeholder `$0` which should have been a normal parameter.
+  bool ShouldPatchPlaceholder0 = CompletingPattern && [CursorKind] {
+    // The process of CCR construction employs `clang::getCursorKindForDecl` to
----------------
Can we factor this out to a namespace scope function `bool shouldPatchPlaceholder0(CodeCompletionResult::ResultKind ResultKind, CXCursorKind CursorKind)`?

Then the code here becomes pretty simple:

```
unsigned CursorSnippetArg = std::numeric_limits<unsigned>::max();
if (shouldPatchPlaceholder0(ResultKind, CursorKind)) {
  CursorSnippetArg = count_if(...);
}
```

And we can combine some of the comments here into a single comment that explains what we are doing:

```
// By default, the final cursor position is at the end of the snippet,
// but we have the option to place it somewhere else using $0.
// If the snippet contains a group of statements, we replace the
// last placeholder with $0 to leave the cursor there, e.g.
//    namespace ${1:name} {
//      ${0:decls}
//    }
// We try to identify such cases using the ResultKind and CursorKind.
```


================
Comment at: clang-tools-extra/clangd/CodeCompletionStrings.cpp:213
       ++SnippetArg;
-      if (SnippetArg == CursorSnippetArg) {
+      if (ShouldPatchPlaceholder0 && SnippetArg == CursorSnippetArg) {
         // We'd like to make $0 a placeholder too, but vscode does not support
----------------
I realized the first part of the condition is now redundant: if `ShouldPatchPlaceholder0 == false`, then `CursorSnippetArg` will have value `std::numeric_limits<unsigned>::max()` and `SnippetArg == CursorSnippetArg` will never be true.


================
Comment at: clang-tools-extra/clangd/unittests/CodeCompletionStringsTests.cpp:30
     Snippet.clear();
-    getSignature(CCS, &Signature, &Snippet, /*RequiredQualifier=*/nullptr,
-                 CompletingPattern);
+    // Note that `getSignature` uses CursorKind to identify if we shouldn't
+    // complete $0 for certain patterns, such as constructors. Passing
----------------
Suggestion: it may be easier to understand if we change the `bool CompletingPattern = false` parameter of this function to `CodeCompletionResult::ResultKind ResultKind = CodeCompletionResult::ResultKind::RK_Declaration`.

(And the call site which passed `CompletingPattern=true` can now pass `ResultKind=RK_Pattern`.)

Then we don't need to refer to historical behaviour.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D145319/new/

https://reviews.llvm.org/D145319