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

Eric Liu via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Fri Mar 22 08:33:00 PDT 2019


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





More information about the cfe-commits mailing list