kadircet added a comment.
sorry for not mentioning in the bug that i was also working on a patch,
D130260 <https://reviews.llvm.org/D130260> seems to be a little bit more
contained and focused on making the check not crash (as we're getting close to
release cut).
WDYT about landing it now, and iterating on this patch afterwards if you still
have cases that it doesn't handle but this patch does? (I can't really see such
cases in hindsight)
================
Comment at: clang-tools-extra/clangd/AST.cpp:844
+ // outer variadic call.
+ const auto LastMatchIdx = FirstMatchIdx + Parameters.size() - 1;
+ if (LastMatchIdx < NumArgs) {
----------------
so in theory this is still a heuristic, and somewhat complicated. what about
just checking if we have "enough parameters" to satisfy the pack expansion (as
i did in D130260)
================
Comment at: clang-tools-extra/clangd/AST.cpp:914
+ static const Expr *unwrapConstructorConversion(const Expr *E) {
+ if (const auto *CBTE = dyn_cast<CXXBindTemporaryExpr>(E)) {
+ E = CBTE->getSubExpr();
----------------
this is already handled by `IgnoreImplicitAsWritten` the underlying function is
using now (landed in https://reviews.llvm.org/D130261)
================
Comment at: clang-tools-extra/clangd/AST.cpp:919
+ const auto *C = CCE->getConstructor();
+ if (!C->isExplicit() && C->getNumParams() == 1) {
+ E = CCE->getArg(0);
----------------
this is checking for constructor being explicit, and not the callexpr itself.
so for example it won't fire up if call is implicit to an explicitly defined
copy constructor.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D130259/new/
https://reviews.llvm.org/D130259
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
