nridge accepted this revision.
nridge added a comment.
This revision is now accepted and ready to land.
Thank you, the updates look good! Please go ahead and merge after addressing
the last minor nits.
================
Comment at: clang-tools-extra/clangd/unittests/CodeCompleteTests.cpp:594
+ Contains(AllOf(named("generic"),
+ signature("<typename T, typename U, typename V>(U, V)"),
+ snippetSuffix("<${1:typename T}, ${2:typename U}>"))));
----------------
It seems a bit inconsistent that the signature includes parameters with default
arguments in the non-call case, and not in the call case. I guess the reason
for this is that in the non-call case, the parameters with defaults become a
`CK_Optional` chunk which clangd adds to the signature
[here](https://searchfox.org/llvm/rev/4c241a9335c3046e418e1b4afc162bc576c072fd/clang-tools-extra/clangd/CodeCompletionStrings.cpp#213-214),
while in the call case the deduced parameters (which include the ones with
defaults) are omitted from the completion string altogether.
It may be more consistent to put the deduced parameters into an optional chunk
as well, and let the consumer of the CodeCompletionString decide whether to use
them?
Anyways, that's an idea for the future, no change needed now.
================
Comment at: clang/lib/Sema/SemaCodeComplete.cpp:1301
+
+ // When completing a non-static member function (and not via
+ // dot/arrow member access) and we're not inside that class' scope,
----------------
nit: let's shorten this comment to just "If we're not inside the scope of the
method's class, it can't be a call"
(The parts about non-static and dot/arrow member access are checked and
explained in `canFunctionBeCalled()`)
================
Comment at: clang/lib/Sema/SemaCodeComplete.cpp:3589
llvm::SmallBitVector Deduced;
- Sema::MarkDeducedTemplateParameters(Ctx, FunTmpl, Deduced);
+ // Avoid running it if this is not a call: We'd emit *all* template
+ // parameters.
----------------
nit: "We'd" --> "We should"
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D156605/new/
https://reviews.llvm.org/D156605
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
