kadircet added a comment.
thanks a lot! haven't checked the tests yet.
================
Comment at: clang-tools-extra/clangd/Hover.cpp:716
+ for (unsigned I = 0; I < CE->getNumArgs() && I < FD->getNumParams(); ++I) {
+ auto *Arg = CE->getArg(I);
+ if (Arg != OuterNode.ASTNode.get<Expr>())
----------------
nit: `Arg` seems to be used only once, consider inlining.
================
Comment at: clang-tools-extra/clangd/Hover.cpp:721
+ // Extract matching argument from function declaration.
+ if (const ParmVarDecl *PVD = FD->getParamDecl(I)) {
+ HI.CalleeArgInfo.emplace(toHoverInfoParam(PVD, Policy));
----------------
nit: braces are usually omitted when "then" part has a single expression.
(applies to multiple other places in the patch)
================
Comment at: clang-tools-extra/clangd/Hover.cpp:726
+ }
+ if (!HI.CalleeArgInfo.hasValue())
+ return;
----------------
nit: you can drop `hasValue`, we mostly make use of the implicit bool
conversion for checking optionals.
================
Comment at: clang-tools-extra/clangd/Hover.cpp:735
+ if (E->getType().isConstQualified()) {
+ PassType.Const = true;
+ }
----------------
isn't this same as `PVD->getType().isConstQualified()`? maybe set it in there?
I think it is more explicit that way, because in theory the `Expr` above is
always the `DeclRefExpr` referencing the current parameter, which is quite
subtle to figure out while looking at this. Or am I missing something?
================
Comment at: clang-tools-extra/clangd/Hover.cpp:760
+ break;
+ default:
+ PassType.Reference = false;
----------------
*sigh* wish we didn't have more than 60 types of implicit casts in c++.
I can see how it is hard to list everything in here and even if you did it
would still be really hard to reason about them.
I can't help myself but wonder though, was there a reasoning when choosing what
to leave out? Because that would help me a lot, and possibly future readers. (I
guess I am also fine with "the list looked comprehensive enough", as we can
always iterate over)
I am a little bit hesitant about the fail policy though(both in here and in
other branches), as I can't prove to myself that being passed as a reference
would be false in all the other cases for example. And showing incorrect
information in here could really end up confusing the user. Unless there's a
clear reasoning behind this failure mode, I would suggest dropping
`CalleeArgInfo` completely to be on the safe side. That way we can improve the
handling for important cases later on, without surfacing possibly wrong
results. WDYT?
================
Comment at: clang-tools-extra/clangd/Hover.h:86
+ // implicit constructor call.
+ bool Reference = true;
+ // True if that reference is false. Never set for non-reference pass.
----------------
why is this true by default ?
================
Comment at: clang-tools-extra/clangd/Hover.h:87
+ bool Reference = true;
+ // True if that reference is false. Never set for non-reference pass.
+ bool Const = false;
----------------
s/false/const/ ?
what would you say to having an enum instead ?
```
enum PassBy {
Value,
Ref,
ConstRef,
};
```
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D81169/new/
https://reviews.llvm.org/D81169
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
