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

Kadir Cetinkaya via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Fri Mar 22 04:31:22 PDT 2019


kadircet marked 15 inline comments as done.
kadircet added inline comments.


================
Comment at: clang-tools-extra/clangd/AST.cpp:88
 static const TemplateArgumentList *
 getTemplateSpecializationArgs(const NamedDecl &ND) {
   if (auto *Func = llvm::dyn_cast<FunctionDecl>(&ND))
----------------
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


================
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:
> why isn't this handled in `getTemplateSpecializationArgLocs`? Add a comment?
it is mentioned in `getTemplateSpecializationArgLocs`, would you rather move the comment here?


================
Comment at: clang/lib/AST/TypePrinter.cpp:1635
 
+static void printArgument(const TemplateArgument &A, const PrintingPolicy &PP,
+                          llvm::raw_ostream &OS) {
----------------
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 ...


================
Comment at: clang/lib/AST/TypePrinter.cpp:1640
+
+static void printArgument(const TemplateArgumentLoc &A,
+                          const PrintingPolicy &PP, llvm::raw_ostream &OS) {
----------------
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.


================
Comment at: clang/lib/AST/TypePrinter.cpp:1643
+  const auto &Kind = A.getArgument().getKind();
+  assert(Kind != TemplateArgument::Null &&
+         "TemplateArgumentKind can not be null!");
----------------
ioeric wrote:
> why? 
> 
> ```
>     /// Represents an empty template argument, e.g., one that has not
>     /// been deduced.
> ```
> It seems legitimate. 
> 
> Since this hot code path, we would want to avoid hard assertion if possible.
Sure, letting TemplateArgument::print handle that


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