zyounan marked 3 inline comments as done. zyounan added a comment. Thank you guys again for the long and meticulous review! :)
For a quick note, here are points I've excerpted from Nathan's reviews that need improvements later: 1. Implement a heuristic that works for function template pointers whose template parameters could be deduced from the declaring type. https://reviews.llvm.org/D156605#inline-1546819 2. Currently, the deducible template parameters are discarded by default; 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. https://reviews.llvm.org/D156605#inline-1548400 ================ 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}>")))); ---------------- nridge wrote: > 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. > It seems a bit inconsistent that the signature includes parameters with > default arguments in the non-call case, and not in the call case. Indeed. If we just want consistent behavior (i.e., both get omitted) for default parameters, it is easy to fix at the moment. Note that 1) the loop for default template params will run iff the bit vector `Deduced` is not empty. 2) the vector would be resized in `Sema::MarkDeducedTemplateParameters` to fit the template params, which has been avoided in my patch to emit all template params for non-call cases. As a result, `LastDeducibleArgument` would always be 0 for non-calls, even with default template params. We can change the `Deduced` to a default initialized N-size bit vector, where N is the size of template params. This way, the default params can be omitted in the loop as desired (by reducing `LastDeducibleArgument` to limit the range it would dump to the CCS chunk later), while non-default params get preserved. > 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? Sounds reasonable. 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
