[PATCH] D154450: [clangd][c++20] Drop first template argument in code completion in some contexts.

[Jens Massberg via Phabricator via cfe-commits]

massberg marked 7 inline comments as done.
massberg added inline comments.


================
Comment at: clang-tools-extra/clangd/CodeComplete.cpp:327
+    return "";
+  return "<" + Signature.substr(FirstComma + 2);
+}
----------------
sammccall wrote:
> I don't love the arithmetic, the repetition of 2 as the length of the string, and the reliance on the space after the comma.
> 
> Maybe: (assuming Signature is a StringRef)
> 
> ```
> [First, Rest] = Signature.split(",");
> if (Rest.empty()) // no comma => one argument => no list needed
>   return "";
> return ("<" + Rest.ltrim()).str();
> ```
Thanks! I agree that the code with StringRefs looks much better.


================
Comment at: clang-tools-extra/clangd/CodeComplete.cpp:484
+      S.Signature = RemoveFirstTemplateArg(S.Signature);
+      S.SnippetSuffix = RemoveFirstTemplateArg(S.SnippetSuffix);
+    }
----------------
sammccall wrote:
> maybe leave a comment:
> 
> // dropping the first placeholder from the suffix will leave a `$2` with no `$1`.
> // However, editors appear to deal with this OK.
> 
> (assuming you've tested this in vscode)
Yes I've tested it with vscode and it looks fine. Why is the numbering of the parameters required?


================
Comment at: clang-tools-extra/clangd/unittests/CodeCompleteTests.cpp:3970
 
-    $other^A<T> auto i = 19;
+    $toplevel^A auto i = 19;
   )cpp");
----------------
sammccall wrote:
> can you also add tests for
> ```
> void abbreviated(A auto x) {}
> 
> template<A auto X> int constrainedNTTP();
> ```
The 
`
void abbreviated(A auto x) {}
`

case doesn't work yet. I will fix it in a follow-up patch.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D154450