ilya-biryukov added a comment.
My biggest concern is that we seem to make output for template instantiation
worse.
There should be a way to stop showing anonymous namespace without introducing
such regressions.
================
Comment at: clang-tools-extra/clangd/Hover.cpp:353
/// Generate a \p Hover object given the type \p T.
HoverInfo getHoverContents(QualType T, const Decl *D, ASTContext &ASTCtx,
+ const SymbolIndex *Index) {
----------------
Not related to this patch, but what is `D` here? Is this getting hover contents
for a type or for a decl?
================
Comment at: clang-tools-extra/clangd/Hover.cpp:356
HoverInfo HI;
- llvm::raw_string_ostream OS(HI.Name);
- PrintingPolicy Policy = printingPolicyForDecls(ASTCtx.getPrintingPolicy());
- T.print(OS, Policy);
- OS.flush();
-
- if (D) {
+ auto FillInHover = [&HI, Index, &ASTCtx](const Decl *D) {
+ if (const auto *ND = llvm::dyn_cast<NamedDecl>(D))
----------------
NIT: could be simplified to
```
if (!D)
D = T->getAsTagDecl();
if (!D)
return HI;
// ... body of FillInHover goes here
```
================
Comment at: clang-tools-extra/clangd/unittests/HoverTests.cpp:365
[](HoverInfo &HI) {
- HI.Name = "class (lambda)";
+ // FIXME: Special case lambdas.
+ HI.Name = "(anonymous class)";
----------------
NIT: could you give an example how you want the output to look like?
================
Comment at: clang-tools-extra/clangd/unittests/HoverTests.cpp:377
[](HoverInfo &HI) {
- HI.Name = "class Foo<int>";
+ HI.Name = "Foo";
HI.Kind = index::SymbolKind::Class;
----------------
`Foo<int>` actually looked better. Do you consider this a regression or is this
intended?
================
Comment at: clang-tools-extra/clangd/unittests/HoverTests.cpp:389
[](HoverInfo &HI) {
- HI.Name = "class Foo<int>";
+ HI.Name = "Foo<int>";
HI.Kind = index::SymbolKind::Class;
----------------
Why does this give different output from the previous example?
I would argue they should both be consistent. Users shouldn't care if there's
an explicit specialization or not.
================
Comment at: clang-tools-extra/clangd/unittests/HoverTests.cpp:533
+ namespace {
+ inline namespace in_ns2 {
+ class Foo {};
----------------
What if anon/inline namespace are interleaves with named ones?
What would it print?
```
namespace a { inline namespace inl { namespace b { namespace { namespace c {
namespace {
struct X {};
}}}}}}
```
Could we test this?
================
Comment at: clang-tools-extra/clangd/unittests/HoverTests.cpp:1196
[](HoverInfo &HI) {
- HI.Name = "class std::initializer_list<int>";
+ // FIXME: Print template instantiation parameters.
+ HI.Name = "initializer_list";
----------------
This looks like a regression. What's stopping us from fixing this right away?
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D71543/new/
https://reviews.llvm.org/D71543
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
