How about add hover information for this expr?
It seems useful to show related information about the class for this expr sometimes.
Details
- Reviewers
sammccall kadircet - Commits
- rG9c328e7afafd: [clangd] Add hover info for `this` expr
Diff Detail
Event Timeline
Thanks, this sounds like a sensible idea. I got a few suggestions for the implementation though.
clang-tools-extra/clangd/Hover.cpp | ||
---|---|---|
610 | what about using the existing getHoverContents(QualType ..) overload instead ? | |
633 | can you handle CXXThisExpr inside this function, instead of an extra branch in the getHover? |
clang-tools-extra/clangd/Hover.cpp | ||
---|---|---|
610 |
| |
610 | Thanks, I tried using getHoverContents(QualType ..) overload and the result looks more simplicity: The origin patch HoverInfo looks like: Both seems reasonable, not sure which one is better. | |
633 | Thanks, here need additional parameter Index *, will update patch soon. |
clang-tools-extra/clangd/Hover.cpp | ||
---|---|---|
610 | I am not sure if there's much value in repeating this and type definition. So I would go with using the QualType overload here. |
clang-tools-extra/clangd/Hover.cpp | ||
---|---|---|
610 | Thanks! It looks more simplicity with PointeeType. Shall we keep the additional information like namespace and template parameters? So we can use getHoverInfo(const NamedDecl ..) overload? |
clang-tools-extra/clangd/Hover.cpp | ||
---|---|---|
610 | i was thinking those would be just repeating what's already available but you seem to be right. especially namespace and template arguments might be useful. | |
631 | let's update the comment too. "Generates hover info for this and evaluatable expressions." | |
685 | can you put this above printExprValue, also the else is unnecessary as branch ends with return, i.e: if(thisexpr) { ... return ..; } if(printExprValue..) { ... return ..; } | |
686 | s/getAsCXXRecordDecl/getAsTagDecl/ | |
clang-tools-extra/clangd/unittests/HoverTests.cpp | ||
2023 | can you add two more test cases, one for a template class and one for a specialization: template <typename T> struct Foo { Foo* bar() { return [[thi^s]]; } }; template <> struct Foo<int> { Foo* bar() { return [[thi^s]]; } }; |
thanks, LGTM!
clang-tools-extra/clangd/Hover.cpp | ||
---|---|---|
643 | nit: return getHoverContents(D, Index); |
Thanks. Update commit to fix the last nit.
It seems that I don't have the commit access...
Yes, I think we should (and I thought we already did!). Sorry for messing this up, I am not sure what made me change my mind in the middle but we should definitely be using QualType printing rather than NamedDecl printing, similar to how we generate hover for autos.
It produces an output like:
namespace ns { template <typename T> struct Foo {}; tempate <> struct Foo<int> { void bar() { thi^s; } }; }
-> const ns::Foo<int>*
which contains both the namespaces and template parameters, including cv-qualifier and is pretty concise. The only downside to Decl printing is we will lose Documentation support, I am not sure how important it is but we can easily augment the output to contain that if it turns out to be important.
getHoverInfo(CXXThisExpr->getType()->getPointeeType(), ...) does not output namespace scope and template parameters without specialization:
while getHoverInfo(CXXThisExpr->getType(), ...) looks weird with partial specialization:
So I did some mix here...
Sorry for the delay here. Kadir is out on vacation.
Yikes - it's a shame reusing our existing type printing doesn't do the right thing, but injected-classname and partial specializations are indeed weird.
I'm tempted to say just to live with the "type-parameter-0-0" nonsense rather than implement the workaround, but it's up to you.
clang-tools-extra/clangd/Hover.cpp | ||
---|---|---|
640 | this needs a comment explaining why we can't reuse existing logic... as far as that can be explained. The body here is complicated, so should probably be factored out into another function. | |
643 | are we missing CV qualifiers? | |
646 | getNamespaceScope isn't going to do the right thing for classes nested in classes. However I think we *don't* want to print the scope here anyway. Generally we just put the name in the hover. I get the argument for following auto, but auto is basically a weird historical exception (it was something like the first hover supported). | |
666 | why are we including default template param values? these are not part of the type. | |
clang-tools-extra/clangd/unittests/HoverTests.cpp | ||
2023 | And a test for const? |
Got it, thanks.
The weird string only happens in injected-classname and partial specialization, maybe we can add a PrintPolicy so that TypePrinter can handle this case? I have tried to extract template arguments by casting the injected-class to ClassTemplatePartialSpecializationDecl, and it seems work well.
Oh, I forgot... we added declaredType() in AST.h - you pass in a TypeDecl and get out a QualType... but it should handle CTPSDecl correctly.
So something like:
Pointee = Thisexpr->getpointeetype
ClassDecl = Pointee->getAsTagDecl
ClassType = declaredType(ClassDecl)
PrettyThisType = ASTContext.getPointerType(QualType(ClassType.getTypePtr(), Pointee.getCVQualifiers()));
then render PrettyThisType?
Sorry, I didn't really address your question. I think the difficulty in making this a printingpolicy is that going from type-parameter-0-0 to the spelled parameter is going from a canonical type to its alias, and isn't really possible in the general case, just in a bunch of special cases. So it's hard to add to the API, because it's a hard promise to live up to.
Thank you, it works like a charm!
For class withou template, getHoverInfo(QualType ...) will add namespace scope by default, so I have to add SuppressScope printpolicy here.
what about using the existing getHoverContents(QualType ..) overload instead ?