[PATCH] D59639: [clangd] Print template arguments helper

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet added inline comments.


================
Comment at: clang-tools-extra/clangd/AST.cpp:133
+    printTemplateArgumentList(OS, *Args, Policy);
+  else if (auto *Cls = llvm::dyn_cast<ClassTemplateSpecializationDecl>(&ND)) {
+    if (const TypeSourceInfo *TSI = Cls->getTypeAsWritten()) {
----------------
ioeric wrote:
> kadircet wrote:
> > ioeric wrote:
> > > why isn't this handled in `getTemplateSpecializationArgLocs`? Add a comment?
> > it is mentioned in `getTemplateSpecializationArgLocs`, would you rather move the comment here?
> I think it would be clearer if we do something like:
> 
> ```
> if (auto *Cls = llvm::dyn_cast<ClassTemplateSpecializationDecl>(&ND)) {
>   // handle this specially because ...
> } else {
>   // use getTemplateSpecializationArgLocs to handle rest of cases.
> }
> 
> ```
I am not performing this in the order you mentioned since `ClassTemplatePartialSpecializationDecl` is also a `ClassTemplateSpecializationDecl`, but if you insist I can change the ordering by adding another condition(which just looks ugly).


================
Comment at: clang-tools-extra/clangd/AST.cpp:149
+    } else {
+      // FIXME: Fix cases when getTypeAsWritten returns null, e.g. friend decls.
+      printTemplateArgumentList(OS, Cls->getTemplateArgs().asArray(), Policy);
----------------
ioeric wrote:
> I'm not quite sure if I understand this FIXME. In this `else` branch, we are handling this case. Do you mean this is not a proper fix and future work is needed here? Could you elaborate?
> 
> One thing that's worth commenting is why we could use `Cls->getTemplateArgs().asArray()` when `Cls->getTypeAsWritten` is null. It's not trivial from the code.
Yes, this should rather be fixed in AST itself. Updating comment to explain the fallback strategy.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D59639