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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
