[PATCH] D142014: [clangd] fix wrong CalleeArgInfo in the hover

[Nathan Ridge via Phabricator via cfe-commits]

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