ioeric 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)) ---------------- can we unify this with `getTemplateSpecializationArgLocs` somehow? it seems that args as written would also be favorable here (see FIXME in line 112) ================ Comment at: clang-tools-extra/clangd/AST.cpp:131 + PrintingPolicy Policy(ND.getASTContext().getLangOpts()); + if (auto Args = getTemplateSpecializationArgLocs(ND)) + printTemplateArgumentList(OS, *Args, Policy); ---------------- Eugene.Zelenko wrote: > Return type is not obvious, so auto should not be used. nit: I'd suggest keeping `{}` for symmetry. ================ 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()) { ---------------- why isn't this handled in `getTemplateSpecializationArgLocs`? Add a comment? ================ Comment at: clang-tools-extra/clangd/AST.cpp:134 + else if (auto *Cls = llvm::dyn_cast<ClassTemplateSpecializationDecl>(&ND)) { + if (const TypeSourceInfo *TSI = Cls->getTypeAsWritten()) { + auto STL = TSI->getTypeLoc().getAs<TemplateSpecializationTypeLoc>(); ---------------- could you add comment/example explaining what's special about this case? ================ Comment at: clang-tools-extra/clangd/AST.h:54 +/// Returns an empty string if type is not a template specialization. +std::string printTemplateArgsAsWritten(const NamedDecl &ND); + ---------------- Maybe `printTemplateSpecializationArgs` since we are only handling template specialization? I think we could drop `AsWritten` from the name. It should be clear to just explain in the comment. ================ Comment at: clang-tools-extra/unittests/clangd/ASTUtilsTests.cpp:16 + +TEST(ASTUtils, PrintTemplateArgs) { + Annotations Test(R"cpp( ---------------- I think this kind of tests would be more readable using `TEST_P` [1] with code and expected arguments as parameters. Up to you though. [1] https://github.com/google/googletest/blob/master/googletest/docs/advanced.md#specifying-names-for-value-parameterized-test-parameters ================ Comment at: clang/lib/AST/TypePrinter.cpp:1635 +static void printArgument(const TemplateArgument &A, const PrintingPolicy &PP, + llvm::raw_ostream &OS) { ---------------- could you add clang tests for these changes? ================ Comment at: clang/lib/AST/TypePrinter.cpp:1643 + const auto &Kind = A.getArgument().getKind(); + assert(Kind != TemplateArgument::Null && + "TemplateArgumentKind can not be null!"); ---------------- 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. 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