[PATCH] D124690: [clangd] add inlay hints for std::forward-ed parameter packs

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

In D124690#3530272 <https://reviews.llvm.org/D124690#3530272>, @upsj wrote:

> I will update this to split up the control flow into a visitor just collecting a single recursion level and the high-level mapping. A general question here would be whether we should do the resolution by parameter or by parameter pack. Doing it by parameter (like I did it right now) is much easier for inlay hints, especially if the pack gets split up into different parts, but that may not necessarily be the case for template functions and more fuzzy cases like signature help/autocomplete.

I'm not sure I have perfectly grasped the distinction you're drawing, but IIUC the dilemma:

- we could query separately where each arg is forwarded to. For multiple forwards, the recursive step is "what param is arg N of this callee ultimately bound to"
- we could query where a range of args is forwarded to. The recursive step is "what params are args [N-M] of this callee ultimately bound to"

Seems like the first is simpler but the second is more efficient. The second also more clearly ensures we don't give inconsistent results for adjacent parameters.
The approach you take now where you walk->cache->query does manage to combine the best of both (though I find it complex in other ways).
This is a tricky tradeoff. My guess is that it's fine to start with the simple param-by-param query, and leave a FIXME on the recursive function that querying a range at a time would be more efficient for large packs.



================
Comment at: clang-tools-extra/clangd/InlayHints.cpp:261
+private:
+  void handleParmVarDeclName(const FunctionDecl *Callee, size_t I) {
+    const auto *Param = Callee->getParamDecl(I);
----------------
upsj wrote:
> sammccall wrote:
> > Unless I'm missing something, going looking in the redecls of the function for a parameter name doesn't seem in scope for this patch.
> > 
> > We don't support it in inlay hints elsewhere, and it's not clear it has anything to do with forwarding functions.
> > Maybe the added complexity is justifiable if this logic can be shared with different functions (hover, signature help) but I don't think it belongs in this patch.
> This was already functionality previously available in `chooseParameterNames`, I thought I would need to do the same thing here, but turns out that I can get the `FunctionDecl` from a `ParmVarDecl`, so this can stay in its previous place.
You're right, sorry!
I think the the version we have is pretty opportunistic: this is a couple of lines of code, so why not?
I don't think we should aim for parity with it, but rather just handle whatever cases are both useful & trivial to implement (in this patch)

> I can get the FunctionDecl from a ParmVarDecl, so this can stay in its previous place.
This sounds good to me - means we can drop this map right? That was the main piece I was concerned with.


================
Comment at: clang-tools-extra/clangd/InlayHints.cpp:314
+        if (auto *FuncCandidate = dyn_cast_or_null<FunctionDecl>(Candidate)) {
+          if (FuncCandidate->getNumParams() == D->getNumArgs()) {
+            if (MatchingDecl) {
----------------
upsj wrote:
> There is probably more generic functionality available for this?
Not AFAIK. There's a bunch of rich info about overload sets and their quality, but it's discarded after Sema and not preserved in the AST I think.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D124690/new/

https://reviews.llvm.org/D124690