sammccall added a comment.
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.
================
Comment at: clang-tools-extra/clangd/Hover.cpp:640
HoverInfo HI;
+ // For `this` expr we currently generate hover with class declaration.
+ if (const CXXThisExpr *CTE = dyn_cast<CXXThisExpr>(E)) {
----------------
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.
================
Comment at: clang-tools-extra/clangd/Hover.cpp:643
+ std::string Name;
+ llvm::raw_string_ostream OS(Name);
+
----------------
are we missing CV qualifiers?
================
Comment at: clang-tools-extra/clangd/Hover.cpp:646
+ const NamedDecl *ND = CTE->getType()->getPointeeType()->getAsTagDecl();
+ const auto NS = getNamespaceScope(ND);
+ if (!NS.empty()) {
----------------
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).
And it's clear that you need namespace qualifiers for auto (it could be
anything!) but less clear you need it here as we're guaranteed to be inside the
type. I'd suggest just dropping it.
================
Comment at: clang-tools-extra/clangd/Hover.cpp:666
+ if (P.Default)
+ OS << " = " << P.Default;
+ }
----------------
why are we including default template param values? these are not part of the
type.
================
Comment at: clang-tools-extra/clangd/unittests/HoverTests.cpp:2023
+ {
+ R"cpp(// this expr
+ namespace ns {
----------------
kadircet wrote:
> 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]]; }
> };
> ```
And a test for const?
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D92041/new/
https://reviews.llvm.org/D92041
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
