[PATCH] D130259: [clangd] fix crash and handle implicit conversions in inlay hints for forwarding functions

[Kadir Cetinkaya via Phabricator via cfe-commits]

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