[PATCH] D71543: [clangd] Fix handling of inline/anon namespaces and names of deduced types in hover

[Ilya Biryukov via Phabricator via cfe-commits]

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