kadircet marked an inline comment as done.
kadircet added a comment.
thanks! that looks really useful, especially knowing when a parameter is being
passed as a ref/val.
================
Comment at: clang-tools-extra/clangd/Hover.cpp:292
+void fillParam(const ParmVarDecl *PVD, HoverInfo::Param &Out,
+ const PrintingPolicy &Policy) {
----------------
why not return a `HoverInfo::Param` instead ?
================
Comment at: clang-tools-extra/clangd/Hover.cpp:525
+ if (const auto *Var = dyn_cast<VarDecl>(D)) {
+ // Check if we are inside CallExpr
----------------
umm, is this a fixme or a leftover? I suppose it is from a previous version of
the patch.
================
Comment at: clang-tools-extra/clangd/Hover.cpp:698
+// information about that argument.
+void maybeAddCalleeArgInfo(const SelectionTree::Node *N, HoverInfo &HI,
+ const PrintingPolicy &Policy) {
----------------
nit: move into anonymous namespace
================
Comment at: clang-tools-extra/clangd/Hover.cpp:700
+ const PrintingPolicy &Policy) {
+ if (!N || !N->outerImplicit().Parent)
+ return;
----------------
nit: extract `N->outerImplicit()` to a variable, it seems to be repeated along.
================
Comment at: clang-tools-extra/clangd/Hover.cpp:713
+ }
+ if (I >= CE->getNumArgs())
+ return;
----------------
i don't think this is necessary, we are baling out if `FD->getNumParams() <= I`
anyways.
================
Comment at: clang-tools-extra/clangd/Hover.cpp:716
+ // Extract matching argument from function declaration.
+ if (const FunctionDecl *FD = CE->getDirectCallee()) {
+ if (FD->isOverloadedOperator() || FD->isVariadic() ||
----------------
nit: put this before arg index finding logic and do an early exit:
```
if(!CE) return;
auto *FD = CE->getDirectCallee();
if(!FD || FD->isOverloaded || variadic..) return;
```
after doing this you can also change the arg index finding logic to:
```
auto *SelectedArg = N->outerImplicit().ASTNode.get<Expr>();
if(!SelectedParam) return; // you can also move this above other exit
conditions.
for (unsigned I = 0; I < min(CE->getNumArgs, FD->getNumParams())...) { // I
suppose these two can be different in presence of default params ?
if (CE->getArg(I) == SelectedArg) {
// do magic;
break;
}
}
``
================
Comment at: clang-tools-extra/clangd/Hover.cpp:717
+ if (const FunctionDecl *FD = CE->getDirectCallee()) {
+ if (FD->isOverloadedOperator() || FD->isVariadic() ||
+ FD->getNumParams() <= I)
----------------
i can see the reason for variadics, but why bail out on overloaded operators?
also can we have some tests for these two?
================
Comment at: clang-tools-extra/clangd/Hover.cpp:787
HI->Value = printExprValue(N, AST.getASTContext());
+ maybeAddCalleeArgInfo(N, *HI, AST.getASTContext().getPrintingPolicy());
} else if (const Expr *E = N->ASTNode.get<Expr>()) {
----------------
this place is getting crowded. maybe we should start passing `N` into
`getHoverContents` and encapsulate the logic in there or something. no action
needed, just thinking out loud.
================
Comment at: clang-tools-extra/clangd/Hover.cpp:893
+
+ if (CalleeArgInfo) {
+ Output.addRuler();
----------------
let's put this after `Size` and before `Documentation` we've been keeping the
documentation and definition as kind of a `footer` for a while now.
that would also drop the need for a ruler.
================
Comment at: clang-tools-extra/clangd/Hover.cpp:897
+ llvm::raw_string_ostream OS(Buffer);
+ OS << "Passed as " << *CalleeArgInfo;
+ Output.addParagraph().appendText(OS.str());
----------------
i bet there will always be some people getting confused no matter what we say
in here (especially since we are not just printing the type, and I totally find
printing param name useful especially in case of literals). But to align with
other types of information we provide maybe say:
`Substitutes: ` ?
================
Comment at: clang-tools-extra/clangd/unittests/HoverTests.cpp:728
+ C c;
+ c.fun([[^a]], b);
+ }
----------------
can you also add a test for a literal ?
================
Comment at: clang-tools-extra/clangd/unittests/HoverTests.cpp:2093
+
+Passed as int arg_a = 7)",
}};
----------------
i find showing the default value a little bit confusing, as user is providing a
different value. i can also see some value in it though, so up to you.
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
