[PATCH] D92977: [clangd] Improve hover and goToDefinition on auto and dectype

[Quentin Chateau via Phabricator via cfe-commits]

qchateau marked 3 inline comments as done.
qchateau added inline comments.


================
Comment at: clang-tools-extra/clangd/XRefs.cpp:689
+        const NamedDecl *D = Deduced->getTypePtr()->getAsTagDecl();
+        if (!D)
+          continue;
----------------
sammccall wrote:
> there are many other types we could resolve to a decl: TypedefType, TemplateTypeParmtype, ...
> If we're only going to handle tag types, it deserves a FIXME comment.
> 
> But we do have  a helper for this. Can you try calling `targetDecl(DynTypedNode::create(Deduced), DeclRelation::TemplatePattern | DeclRelation::Alias)` instead of `Deduced->getTypePtr()->getAsTagDecl()`?
> 
> That should handle more cases, at a minimum, this case sholud pass:
> ```
> using [[T]] = int;
> T x;
> ^auto y = x;
> ```
> 
> (I see you have a test case that aliases backed by tag types are resolved to the underlying tag decl, this change would offer the alias instead, which is more consistent with our current go-to-definition. You could also offer both by passing `DeclRelation::TemplatePattern | DeclRelation::Alias | DeclRelation::Underlying`... but I think we should value consistency here)
`locateSymbolForType` uses `targetDecl`. Thanks for the tip, that's exactly what I was looking for.

I also agree with you that go-to-definition should go to the alias instead of the underlying type for consistency, but using `targetDecl(DynTypedNode::create(Deduced), DeclRelation::TemplatePattern | DeclRelation::Alias)` is not enough to resolve this consistency issue: `getDeducedType` already returns the underlying type. My current knowledge of the clangd code is nou sufficient to understand why, but the root of the problem seem to lie in the `DeducedTypeVisitor` class.

I removed the test that tested the undesirable behavior, should I open a bug report for `getDeducedType` that should not "go through" aliases ? In which case, what's the right place for that ? Github ?


================
Comment at: clang-tools-extra/clangd/unittests/XRefsTests.cpp:669
+
+      R"cpp(// auto on template class with forward declared class
+        template<typename T> class [[Foo]] {};
----------------
sammccall wrote:
> There's a scalability problem with testing every corner of the C++ language (there are lots!) with every feature. Generally I prefer to cover some important/weird/bugfix cases here, but to cover as much as possible with unittests of the underlying functions.
> 
> In this case, that would mean
>  - moving tests that are about `auto` in different contexts to ASTTests.cpp (current test coverage there is poor).
>  - (assuming we can use `targetDecl()`) moving tests that are about which decl a type should resolve to to `FindTargetTests.cpp` - but current coverage there is pretty good so we can probably just drop many of them.
I added the relevant tests to `ASTTests.cpp` but I did not remove the tests here for the moment. I've always had the habit to keep tests that are already written, even if redundant.

If you'd like me to remove some of them, I'd say the only tests that are not redundant with the ones in `ASTTests.cpp` are the ones that test template specializations. Shoud I keep only these ?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92977