[clangd] Print arguments in template specializations

[ilya-biryukov]

clang-tools-extra/clangd/AST.cpp

57 ↗

(On Diff #190591)

For ClassTemplateSpecializationDecl, could we try extracting the arguments from the result of getTypeAsWritten?

clang/lib/AST/TypePrinter.cpp

1636 ↗

(On Diff #190591)

NIT: clang-format the diff

1644 ↗

(On Diff #190591)

Maybe print the result of getTypeLoc() here, if it's available?
Would produce results closer to the ones written in the code.

[kadircet]

• Address comments
• Add template specializations to fuzzyFind results

[kadircet]

clang/lib/AST/TypePrinter.cpp

1644 ↗

(On Diff #190591)

unfortunately it is not available.

[kadircet]

• Get rid of debug printing
• Update tests

[ilya-biryukov]

Mostly nits, but could you elaborate why can't we print the type-loc when printing the template argument (see the corresponding comment)?

clang-tools-extra/clangd/AST.cpp

66 ↗

(On Diff #190624)

NIT: add a note that we return null for ClassTemplateSpecializationDecl because we can't construct an ArrayRef for it

84 ↗

(On Diff #190624)

NIT: use SmallVector<8> or some other small-enough number to avoid most allocs.

clang-tools-extra/unittests/clangd/IndexTests.cpp

18 ↗

(On Diff #190624)

NIT: maybe remove it? clangd keeps adding those, but I don't think we actually want it: gmock.h should be enough

clang-tools-extra/unittests/clangd/SymbolCollectorTests.cpp

417 ↗

(On Diff #190624)

Does it mean typing bool could potentially match vector<bool> in workspaceSymbols now?
If that's the case we might start producing some noise.

clang/lib/AST/TypePrinter.cpp

1646 ↗

(On Diff #190624)

Maybe simplify the switch to:

if (A.getKind() == TemplateArgument::ArgKind::Type) {
    A.getTypeSourceInfo()->getType().print(OS, PP);
    return;
}
A.getArgument().print(PP, OS);

1644 ↗

(On Diff #190591)

you mean the function to print a type loc or the type loc itself?

[kadircet]

clang-tools-extra/clangd/AST.cpp

84 ↗

(On Diff #190624)

calling reserve beforehand

clang-tools-extra/unittests/clangd/IndexTests.cpp

18 ↗

(On Diff #190624)

Should we add IWYU pragmas to those files? https://github.com/google/googlemock/blob/master/googlemock/include/gmock/gmock-generated-matchers.h

clang-tools-extra/unittests/clangd/SymbolCollectorTests.cpp

417 ↗

(On Diff #190624)

unfortunately yes it does, what do you suggest? it seems like we can perform a "isprefix" check for symbols with template specs kind?

clang/lib/AST/TypePrinter.cpp

1644 ↗

(On Diff #190591)

function to print a typeloc

[kadircet]

• Address some of the comments

[kadircet]

clang/lib/AST/TypePrinter.cpp

1646 ↗

(On Diff #190624)

It was rather to catch any changes in the ArgKind at compile-time, still can do if you think this should not cause any problems

[ilya-biryukov]

The only important comment is about test coverage

clang-tools-extra/clangd/AST.cpp

84 ↗

(On Diff #190624)

SmallVector is even better because it keeps the elements on the stack and won't need to do dynamic allocations (most of the time).
My suggestion would be to use it here anyway.

Up to you, though.

clang-tools-extra/unittests/clangd/IndexTests.cpp

18 ↗

(On Diff #190624)

I think we should, but some projects don't like those pragmas in their headers.
Trying to add this won't hurt for sure!

clang-tools-extra/unittests/clangd/SymbolCollectorTests.cpp

417 ↗

(On Diff #190624)

Let's land this and see if this creates too much noise.
If it will, we might want to drop the argument lists from the words we match and only keep them for presentation to the user.

But let's not worry about this too much for now, the current behavior LG unless proven otherwise :-)

clang/lib/AST/TypePrinter.cpp

1655 ↗

(On Diff #190642)

Now that you mentioned other kinds of parameters, could you add tests for other kinds of parameters?
In particular, I expect we'll definitely need to handle packs here and probably all other kinds of arguments too.

• Non-type and template template parameters.

template <int I, int J, template<class> class TT, template <class> class JJ>
struct Foo {};

template <int X, template <class> class UU>
struct Foo<X, X, UU, UU> {};

• Parameter packs.

template <class ...T>
struct Bar {};

template <class T>
struct Bar<T, T> {};

template <int ...I>
struct Baz {};

template <int I>
struct Baz<I, I> {};

template <template <class> class...TT>
struct Qux {};

template <template <class> class TT>
struct Qux<TT, TT> {};

1646 ↗

(On Diff #190624)

I think listing all the cases here does not actually buy us anything since we call a function that should somehow print the arguments anyway.

But up to you.

[ilya-biryukov]

clang/lib/AST/TypePrinter.cpp

1644 ↗

(On Diff #190591)

Thanks for the clarification.
Intuitively, I've expected it to be there, but it seems that we can only print the types.

[kadircet]

• Add more tests
• Replace switch with if
• Use SmallVector

clang/lib/AST/TypePrinter.cpp

1655 ↗

(On Diff #190642)

I've already had tests for these cases, but adding a few more for partial specs as well.

[ilya-biryukov]

LGTM with a few NITs.
Thanks for fixing this!

clang-tools-extra/clangd/AST.cpp

86 ↗

(On Diff #190800)

NIT: maybe inline TL?

if (auto STL = Cls->getTypeAsWritten()->getTypeLoc().getAs<TemplateSpecializationTypeLoc>()) {
  /// ...
}

89 ↗

(On Diff #190800)

Tiny NIT: prefer preincrement

clang/lib/AST/TypePrinter.cpp

1635 ↗

(On Diff #190800)

NIT: consider also adding a test in the clang code.
Some developers don't build tools-extra.

[ilya-biryukov]

clang/lib/AST/TypePrinter.cpp

1635 ↗

(On Diff #190800)

It seems the amount of test infrastructure for running this in clang might be overwhelming.
It's just not worth it probably, having a test in clangd is good enough!

[kadircet]

• Address comments

lib/AST/TypePrinter.cpp

 
 static
 const TemplateArgument &getArgument(const TemplateArgument &A) { return A; }
 
 static const TemplateArgument &getArgument(const TemplateArgumentLoc &A) {
   return A.getArgument();
 }
 
+static void printArgument(const TemplateArgument &A, const PrintingPolicy &PP,
+                          llvm::raw_ostream &OS) {
+  A.print(PP, OS);
+}
+
+static void printArgument(const TemplateArgumentLoc &A,
+                          const PrintingPolicy &PP, llvm::raw_ostream &OS) {
+  const auto &Kind = A.getArgument().getKind();
+  assert(Kind != TemplateArgument::Null &&
+         "TemplateArgumentKind can not be null!");
+  if (Kind == TemplateArgument::ArgKind::Type)
+    return A.getTypeSourceInfo()->getType().print(OS, PP);
+  return A.getArgument().print(PP, OS);
+}
+
 template<typename TA>
 static void printTo(raw_ostream &OS, ArrayRef<TA> Args,
                     const PrintingPolicy &Policy, bool SkipBrackets) {
   const char *Comma = Policy.MSVCFormatting ? "," : ", ";
   if (!SkipBrackets)
     OS << '<';
 
   bool NeedSpace = false;
   bool FirstArg = true;
   for (const auto &Arg : Args) {
     // Print the argument into a string.
     SmallString<128> Buf;
     llvm::raw_svector_ostream ArgOS(Buf);
     const TemplateArgument &Argument = getArgument(Arg);
     if (Argument.getKind() == TemplateArgument::Pack) {
       if (Argument.pack_size() && !FirstArg)
         OS << Comma;
       printTo(ArgOS, Argument.getPackAsArray(), Policy, true);
     } else {
       if (!FirstArg)
         OS << Comma;
-      Argument.print(Policy, ArgOS);
+      printArgument(Arg, Policy, ArgOS);
     }
     StringRef ArgString = ArgOS.str();
 
     // If this is the first argument and its string representation
     // begins with the global scope specifier ('::foo'), add a space
     // to avoid printing the diagraph '<:'.
     if (FirstArg && !ArgString.empty() && ArgString[0] == ':')
       OS << ' ';