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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
