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