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

[Eric Liu via Phabricator via cfe-commits]

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