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

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

Cool! This makes sense, and is much cheaper than actually working out how to render the abbreviated arg list and store it in the index.

Nice that TopLevel gives us some of these contexts, but I suspect it's not all. (If not, it's fine to handle just this case for now and leave the others as tests showing the bad behavior with a FIXME)



================
Comment at: clang-tools-extra/clangd/CodeComplete.cpp:321
+// If Signature only contains a single argument an empty string is returned.
+std::string RemoveFirstTemplateArg(const std::string &Signature) {
+  if (Signature.empty() || Signature.front() != '<' || Signature.back() != '>')
----------------
RemoveFirst => removeFirst


================
Comment at: clang-tools-extra/clangd/CodeComplete.cpp:321
+// If Signature only contains a single argument an empty string is returned.
+std::string RemoveFirstTemplateArg(const std::string &Signature) {
+  if (Signature.empty() || Signature.front() != '<' || Signature.back() != '>')
----------------
sammccall wrote:
> RemoveFirst => removeFirst
(if this function takes StringRef instead you get nicer APIs like split, and less copying)


================
Comment at: clang-tools-extra/clangd/CodeComplete.cpp:327
+    return "";
+  return "<" + Signature.substr(FirstComma + 2);
+}
----------------
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();
```


================
Comment at: clang-tools-extra/clangd/CodeComplete.cpp:481
+
+    // Hack to remove first template argument in some special cases.
+    if (IsConcept && ContextKind == CodeCompletionContext::CCC_TopLevel) {
----------------
more important to say why rather than what: which cases?

```
/// When a concept is used as a type-constraint (e.g. `Iterator auto x`),
/// and in some other contexts, its first type argument is not written.
/// Drop the parameter from the signature.
```


================
Comment at: clang-tools-extra/clangd/CodeComplete.cpp:484
+      S.Signature = RemoveFirstTemplateArg(S.Signature);
+      S.SnippetSuffix = RemoveFirstTemplateArg(S.SnippetSuffix);
+    }
----------------
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)


================
Comment at: clang-tools-extra/clangd/unittests/CodeCompleteTests.cpp:3964
 
+    template<U>
+    int bar() requires $other^A;
----------------
this doesn't look valid - what is U here?
shouldn't `requires A` be `requires A<...>?

(generally if we're mixing completion cases in one file it's important that they're valid so subsequent ones parse correctly - also to not confuse the reader)


================
Comment at: clang-tools-extra/clangd/unittests/CodeCompleteTests.cpp:3968
     template<class T>
-    concept b = $other^A<T> && $other^sizeof(T) % 2 == 0 || $other^A<T> && sizeof(T) == 1;
+    concept b = $other^A && $other^sizeof(T) % 2 == 0 || $other^A && sizeof(T) == 1;
 
----------------
I think it might be worth renaming "other" to "expr", it seems to more clearly describe all the situations here


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

template<A auto X> int constrainedNTTP();
```


================
Comment at: clang-tools-extra/clangd/unittests/TestIndex.h:38
 // Create a C++20 concept symbol.
-Symbol conceptSym(llvm::StringRef Name);
+Symbol conceptSym(llvm::StringRef Name, llvm::StringRef Signature = "",
+                  llvm::StringRef CompletionSnippetSuffix = "");
----------------
this diverges from how the other helpers work: they're mostly creating minimal skeletons and encapsulate the mess of synthesizing USRs.

Instead of extending the function here, we can assign the appropriate fields on the returned symbol at the callsite (or add a local helper to do so)

(objcSym is also out-of-line with the pattern here, but we shouldn't follow it off the rails)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D154450