ioeric added inline comments.
================
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);
----------------
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.
================
Comment at: clang-tools-extra/clangd/AST.cpp:88
static const TemplateArgumentList *
getTemplateSpecializationArgs(const NamedDecl &ND) {
if (auto *Func = llvm::dyn_cast<FunctionDecl>(&ND))
----------------
kadircet wrote:
> ioeric wrote:
> > can we unify this with `getTemplateSpecializationArgLocs` somehow?
> >
> > it seems that args as written would also be favorable here (see FIXME in
> > line 112)
> See D59641
thought?
================
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()) {
----------------
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.
}
```
================
Comment at: clang/lib/AST/TypePrinter.cpp:1635
+static void printArgument(const TemplateArgument &A, const PrintingPolicy &PP,
+ llvm::raw_ostream &OS) {
----------------
kadircet wrote:
> ioeric wrote:
> > could you add clang tests for these changes?
> We've also discussed this in the original comment and decided infrastructure
> necessary was too overwhelming. First we need to invoke compiler and get the
> AST, then write a Visitor to fetch relevant decls and only after that call
> printTemplateArguments ...
No test at all is concerning... I think there are lit tests for AST printing in
test/AST/. Is it possible to have some of those?
================
Comment at: clang/lib/AST/TypePrinter.cpp:1640
+
+static void printArgument(const TemplateArgumentLoc &A,
+ const PrintingPolicy &PP, llvm::raw_ostream &OS) {
----------------
kadircet wrote:
> ioeric wrote:
> > It's unclear to me what the new behavior is with changes in this file.
> > Could you add comment?
> >
> > It might make sense to split the clang change into a separate patch and get
> > folks who are more familiar to take a look.
> it just prevents erasure from TypeLoc to Type when printing the argument, so
> that we can do a fine-grained printing if Location is already there.
could you add this into comment?
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D59639/new/
https://reviews.llvm.org/D59639
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
