[PATCH] D96751: [clangd] Populate detail field in document symbols

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

This looks really good. Only thing i'd really think about changing in behavior is dropping the template params.
Please go ahead and upload the unittests!

(And thanks for including the diff context)



================
Comment at: clang-tools-extra/clangd/FindSymbols.cpp:184
+  llvm::raw_string_ostream OS(Detail);
+  if (const auto *TP = ND.getDescribedTemplateParams()) {
+    TP->print(OS, Ctx, P);
----------------
As a tradeoff between brevity and information, we might consider printing `template` but not the param list.

Particularly for functions this is useful, as template params are mostly types repeated in the signature:
```
- DenseMap | template <typename InputIt> void (const InputIt&, const InputIt&)
+ DenseMap | template void (const InputIt&, const InputIt&)
```

But I think this may be fine for classes as well:
```
- DenseMap | template <typename KeyT, typename ValueT, typename KeyInfoT = ...> class
+ DenseMap | template class
```


================
Comment at: clang-tools-extra/clangd/FindSymbols.cpp:188
+  if (const auto *VD = dyn_cast<ValueDecl>(&ND)) {
+    VD->getType().print(OS, P);
+  } else if (const auto *TD = dyn_cast<TagDecl>(&ND)) {
----------------
It might not makes sense to print the placeholder DependentType, types that contain undeduced `auto`, and maybe some other cases.

(This can definitely be a fixme comment for later - it'll be hard to find exactly the right cases in one pass)



================
Comment at: clang-tools-extra/clangd/FindSymbols.cpp:192
+  } else if (const auto *TD = dyn_cast<TypedefNameDecl>(&ND)) {
+    OS << "type alias";
+  } else if (const auto *CD = dyn_cast<ConceptDecl>(&ND)) {
----------------
What you have here is probably best, just thinking out loud...

we could also consider including the underlying type like `= int` but I suspect the difference between ValueDecl is too subtle and sometimes the types are really long anyway.


================
Comment at: clang-tools-extra/clangd/FindSymbols.cpp:194
+  } else if (const auto *CD = dyn_cast<ConceptDecl>(&ND)) {
+    CD->getTemplateParameters()->print(OS, Ctx, P);
+    OS << "concept";
----------------
(if we drop the template params then I don't think we need to even print "template" in this case because concepts are always templates)


================
Comment at: clang-tools-extra/clangd/FindSymbols.cpp:197
+  }
+  return Detail;
+}
----------------
nit: either `OS.flush(); return Detail;` or equivalently `return std::move(OS.str());`

(Otherwise whether OS's buffer gets flushed into the right string or not is fragile, I believe relying on move elision)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D96751/new/

https://reviews.llvm.org/D96751