nridge added a subscriber: adamcz. nridge added a comment. Thanks for the patch!
I had a look at the code history, and it looks like the original reason for not using the param decl's type to determine the passing mode was (based on this comment <https://reviews.llvm.org/D81169#inline-761047>) to handle cases like this correctly: struct C { C(int&); }; void foo(const C&); int main() { int y; foo(y); } However, your patch keeps this case working by keeping the check for a `CXXConstructExpr` and using the constructor's parameter decl instead (and this scenario has test coverage in the test `Hover.CallPassType`), so this seems fine to me, and indeed a nice simplification. That said, I will cc @adamcz and @kadircet (as author and reviewer of the original patch) to see if they have any concerns about this refactoring breaking any other cases. ================ Comment at: clang-tools-extra/clangd/Hover.cpp:955 +HoverInfo::PassType::PassMode getPassMode(QualType QT) { + if (QT->isReferenceType()) { ---------------- nit: name the parameter `ParmType`, so it's clear whose type it's expecting ================ Comment at: clang-tools-extra/clangd/Hover.cpp:1032 + } else { + PassType.PassBy = getPassMode(CD->getParamDecl(0)->getType()); PassType.Converted = true; ---------------- Please explicily check `getNumParams()` before calling `getParamDecl(0)`. (Even though the constructpr //should// have at least one parameter, we don't want to risk crashing in the case of invalid code or some other unexpected situation.) Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D142014/new/ https://reviews.llvm.org/D142014 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
