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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
