[PATCH] D100742: [clangd] Parameter hints for dependent calls

[Sam McCall via Phabricator via cfe-commits]

sammccall accepted this revision.
sammccall added inline comments.
This revision is now accepted and ready to land.


================
Comment at: clang-tools-extra/clangd/InlayHints.cpp:56
+    auto CalleeDecls = Resolver->resolveCalleeOfCallExpr(E);
+    if (CalleeDecls.empty())
+      return true;
----------------
should we conservatively make this size != 1?
e.g. if the callee ends up being an OverloadExpr of some kind, picking the first overload seems pretty arbitrary and maybe misleading.


================
Comment at: clang-tools-extra/clangd/unittests/InlayHintTests.cpp:247
 
-TEST(ParameterHints, DependentCall) {
-  // FIXME: This doesn't currently produce a hint but should.
+TEST(ParameterHints, DependentCalls) {
   assertParameterHints(R"cpp(
----------------
can we add a test involving overloads?

i think:
```
void foo(int anInt);
void foo(double aDouble);
template <class T> go {
  foo(T{}); // expect no hints
}
```


================
Comment at: clang-tools-extra/clangd/unittests/InlayHintTests.cpp:264
+        // FIXME: This one does not work yet.
+        A<T>::static_member($par3[[t]]);
       }
----------------
nridge wrote:
> This is an interesting case. Clang builds a `CXXDependentScopeMemberExpr` for this callee, but `HeuristicResolver` [currently assumes](https://searchfox.org/llvm/rev/92880ab7a2b2145f0605f367cd6d53d6892903c3/clang-tools-extra/clangd/HeuristicResolver.cpp#108) that such expressions are only built for non-static member accesses (since, for static member accesses, clang usually builds a `DependentScopeDeclRefExpr` instead).
> 
> The `CXXDependentScopeMemberExpr` is created [here](https://searchfox.org/llvm/rev/92880ab7a2b2145f0605f367cd6d53d6892903c3/clang/lib/Sema/SemaTemplate.cpp#757), and I note the dependence on whether the //enclosing context// is an [instance method](https://searchfox.org/llvm/rev/92880ab7a2b2145f0605f367cd6d53d6892903c3/clang/lib/Sema/SemaTemplate.cpp#750). I guess it must think that, after instantiation, `A<T>` could turn out to be a base class, and thus this could be a "non-static member access with qualifier".
> 
> I don't see anything obvious on `CXXDependentScopeMemberExpr` that would let us tell apart "definitely a non-static member" from "maybe static, maybe non-static", so I guess the appropriate solution is to drop the `NonStaticFilter` [here](https://searchfox.org/llvm/rev/92880ab7a2b2145f0605f367cd6d53d6892903c3/clang-tools-extra/clangd/HeuristicResolver.cpp#108) altogether?
> I guess it must think that, after instantiation, A<T> could turn out to be a base class, and thus this could be a "non-static member access with qualifier".

Argh, C++ is so tricky :-( That sounds plausible to me.

> drop the NonStaticFilter here altogether

Yeah. The other thing is that `some_instance.some_static_member` is perfectly legal I think? So the NonStaticFilter is probably not correct anyway.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D100742