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