sammccall added a comment.
Thanks for doing this! Go-to-def-on-auto has been bugging me, it'll be great to
have it :-)
Overall the patch looks really solid, with nice tests. The one thing I'd
suggest is splitting the two features (hover and go-to-def) into separate
patches.
e.g. here, I'm not totally sure about the hover change yet, and want to think
and get some input from others. But I don't want to delay the go-to-definition
part. So I've left comments on the go-to-definition changes, and if you want to
drop hover from this patch and upload it as a separate review, that'd be really
helpful.
---
My concern with hover is mostly around consistency and clarity. In isolation,
the new hover certainly looks better than the old one for structs (e.g. syntax
highligthing!).
However now we have a different style for struct vs non-struct types, and it's
not just a surface thing: for structs we show the declaration, but many types
have no declaration to show. I'm not sure how you can show a hover for `auto =
char *const*` or so that feels consistent with this one.
We should also consider consistency and clarity with different types of `auto`.
Currently our hover for undeduced auto is just "auto", and our hover for auto
params is a mess. It would be nice about this distinction, as when the same
keyword has multiple meanings people can conflate them.
One solution would be to make `auto` the name, and the type the definition.
This provides less information (but I suspect that information is often noise):
### deduced-type `auto`
---
struct cv::Ptr<cv::detail::ChannelsCompensator>
(For context, the `auto` hover was one of the first implemented. Later we went
back and added some structure (type/name/definition/documentation) but the old
`auto` hover was mostly shoehorned into this. So if we're going to change auto
hover, we shuold consider trying to make it fit this schema, too)
================
Comment at: clang-tools-extra/clangd/XRefs.cpp:685
+
+ if (Tok.kind() == tok::kw_auto || Tok.kind() == tok::kw_decltype) {
+ if (auto Deduced = getDeducedType(AST.getASTContext(), Tok.location())) {
----------------
This deserves a high-level comment: // go-to-definition on auto should find the
definition of the deduced type, if possible
================
Comment at: clang-tools-extra/clangd/XRefs.cpp:686
+ if (Tok.kind() == tok::kw_auto || Tok.kind() == tok::kw_decltype) {
+ if (auto Deduced = getDeducedType(AST.getASTContext(), Tok.location())) {
+ Deduced = Deduced->getNonReferenceType();
----------------
I think we should pull the contents of this if() into a function
findSymbolForType or so.
LSP has a `textDocument/typeDefinition` request that could also make use of
this logic.
(You don't need to implement that, and I'm very much in favor of having this
`textDocument/definition` be the do-what-i-mean swiss army knife like this
patch does. But I'd just like to factor this code with potential reuse in mind)
================
Comment at: clang-tools-extra/clangd/XRefs.cpp:689
+ const NamedDecl *D = Deduced->getTypePtr()->getAsTagDecl();
+ if (!D)
+ continue;
----------------
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)
================
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]] {};
----------------
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.
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
