libclang: Restore the CXXRecordDecl path for clang_Type_getNumTemplateArguments and clang_Type_getTemplateArgumentAsType

[emilio]

Summary

Since the new path isn't always a superset of the old (see the test), and I want to keep old code working.

[emilio]

Updated to add a missing null check, and remove unnecessary diff noise.

[emilio]

Hi, @EricWF or @akyrtzi, could I get this reviewed?

It's fallout from https://reviews.llvm.org/D26663, and I don't know how often there are llvm/libclang releases, but I'm afraid code using libclang might break without this patch, and I'd be really ashamed if that happened (since it'd be totally my fault :/)

Thanks in advance :)

[emilio]

(Also, please don't rush if I'm wrong being concerned about releases or similar, I'm fairly ignorant in that regard)

[akyrtzi]

When I try just the test case (without the other changes), it doesn't fail when using r288438; so I'm not sure whether the test is actually testing the regression.

[emilio]

Hmm... You're right.

This happens when looking in the canonical type of a given type instead of the type itself, which is a CXType_Record... I'll find a way to test it.

[emilio]

Now with proper tests :)

[EricWF]

Sorry a bad Herald regex added me as a reviewer.

[skalinichev]

Please, integrate test cases from D27384 into this patch and make sure that tests still pass.

[skalinichev]

Some suggestions in D27384 from @compnerd also apply to your patch (at least about using auto), so you should probably fix it too.

[skalinichev]

test/Index/keep-going.cpp

22 ↗

(On Diff #80092)

Why do you print canonicaltemplateargs before templateargs, shouldn't it be the other way around? Given that we first print type and only then canonicaltype it would make more sense IMO.

test/Index/print-type.cpp

107

That's weird why does templateargs has 3 arguments, but canonicaltemplateargs only 1?

[emilio]

Thanks for the comments!

Will address them ASAP and post back.

test/Index/keep-going.cpp

22 ↗

(On Diff #80092)

Seemed easier to keep canonicaltemplateargs right after canonicaltypekind, but surely I can move templateargs before canonicaltype.

test/Index/print-type.cpp

107

It's a template parameter pack:

template <typename... T>
struct Qux { };

[emilio]

test/Index/print-type.cpp

107

Hmm, though you're right it may be better if it'd be expanded. I think it's kind of out of scope here (since it's a limitation that was before my patch), but if it's fine as a followup I'll try to address it after fixing this.

[emilio]

Updated per comments, let me know if I should do anything else.

I didn't do the return MakeCXType(<ternary>, ...) thing since I think it was a bit less clear, but I can do that if needed.

Thanks!

[skalinichev]

test/Index/print-type.cpp

107

I see, but it shouldn't be so difficult to add support for it. Seems like all you have to do is check if it's a template parameter pack and then use pack_size() and getPackAsArray() accordingly.

Anyway if you don't want to implement it now, just add a TODO to the clang_Type_getNumTemplateArguments, that should suffice for now I think.

[emilio]

Updated per comments. Not super happy with making these functions linear, but given template arguments aren't that numerous usually, it's probably acceptable.

[skalinichev]

tools/libclang/CXType.cpp

923

static

926–927

Maybe llvm::Optional can help here a bit?

942

static

946

Are you sure this can happen (one TemplateArgument::Pack can be inside another one)? If so you should definitely add a test case for it, otherwise let's drop this recursion altogether and use pack_size() instead.

962

static

965

Use llvm::Optional instead

989

Ty -> QT

[emilio]

Thanks for the review, good catches :)

Updated again.

[skalinichev]

tools/libclang/CXType.cpp

965

No, that's not what I meant.

Optional<QualType> FindTemplateArgumentTypeAt(...)

966

Now that GetTemplateArgumentArraySize doesn't use recursion anymore, I see no reason why FindTemplateArgumentTypeAt should. It only makes code harder to understand without real necessity

[emilio]

tools/libclang/CXType.cpp

965

I did it this way in order to be able to distinguish "index not found" to "parameter is not a type", but ok, will update :)

966

Fair enough, I found it easier to read than the alternative, but same, I'll update.

[emilio]

Actually it was not so bad, thanks @skalinichev :)

[emilio]

Now without a really dumb off-by-one, hidden by the fact that we always indexed in-range and packs are only the last element.

[emilio]

@skalinichev ping? Anything else I need to do to get this landed?

[akyrtzi]

Committed via r289995, thanks!

test/Index/print-type.cpp

 
 struct TypeAliasUser { TypeAlias<int> foo; };
 
+template<typename T>
+struct Foo {
+  T* ptr;
+};
+
+typedef Foo<int> IntFoo;
+
 // RUN: c-index-test -test-print-type %s -std=c++14 | FileCheck %s
 // CHECK: Namespace=outer:1:11 (Definition) [type=] [typekind=Invalid] [isPOD=0]
 // CHECK: ClassTemplate=Foo:4:8 (Definition) [type=] [typekind=Invalid] [isPOD=0]

 // CHECK: TemplateTypeParameter=T:59:20 (Definition) [type=T] [typekind=Unexposed] [canonicaltype=type-parameter-0-0] [canonicaltypekind=Unexposed] [isPOD=0]
 // CHECK: FieldDecl=foo:62:39 (Definition) [type=TypeAlias<int>] [typekind=Unexposed] [canonicaltype=outer::Qux<int>] [canonicaltypekind=Record] [templateargs/1= [type=int] [typekind=Int]] [isPOD=1]
 // CHECK: TemplateRef=TypeAlias:60:1 [type=] [typekind=Invalid] [isPOD=0]
+// CHECK: ClassTemplate=Foo:65:8 (Definition) [type=] [typekind=Invalid] [isPOD=0]
+// CHECK: TemplateTypeParameter=T:64:19 (Definition) [type=T] [typekind=Unexposed] [canonicaltype=type-parameter-0-0] [canonicaltypekind=Unexposed] [isPOD=0]
+// CHECK: TypedefDecl=IntFoo:69:18 (Definition) [type=IntFoo] [typekind=Typedef] [canonicaltype=Foo<int>] [canonicaltypekind=Record] [templateargs/1= [type=int] [typekind=Int]] [isPOD=0]
+// CHECK: TemplateRef=Foo:65:8 [type=] [typekind=Invalid] [isPOD=0]

tools/libclang/CXType.cpp

   QualType T = GetQualType(CT);
   if (T.isNull())
     return -1;
-  const TemplateSpecializationType *Specialization =
-    T->getAs<TemplateSpecializationType>();
-  if (!Specialization)
-    return -1;
-  return Specialization->template_arguments().size();
+
+  if (const TemplateSpecializationType *Specialization =
+      T->getAs<TemplateSpecializationType>()) {
+    return Specialization->template_arguments().size();
+  }
+
+  if (const CXXRecordDecl *RecordDecl = T->getAsCXXRecordDecl()) {
+    const ClassTemplateSpecializationDecl *TemplateDecl =
+      dyn_cast<ClassTemplateSpecializationDecl>(RecordDecl);
+    if (TemplateDecl)
+      return TemplateDecl->getTemplateArgs().size();
+  }
+
+  return -1;
 }
 
 CXType clang_Type_getTemplateArgumentAsType(CXType CT, unsigned i) {

   if (T.isNull())
     return MakeCXType(QualType(), GetTU(CT));
 
-  const TemplateSpecializationType *Specialization =
-    T->getAs<TemplateSpecializationType>();
-  if (!Specialization)
-    return MakeCXType(QualType(), GetTU(CT));
-  auto TA = Specialization->template_arguments();
-  if (TA.size() <= i)
+  ArrayRef<TemplateArgument> Args;
+  if (const TemplateSpecializationType *Specialization =
+      T->getAs<TemplateSpecializationType>()) {
+    Args = Specialization->template_arguments();
+  } else if (const CXXRecordDecl *RecordDecl = T->getAsCXXRecordDecl()) {
+    const ClassTemplateSpecializationDecl *TemplateDecl =
+      dyn_cast<ClassTemplateSpecializationDecl>(RecordDecl);
+    Args = TemplateDecl->getTemplateArgs().asArray();
+  }
+
+  if (Args.size() <= i)
     return MakeCXType(QualType(), GetTU(CT));
-  const TemplateArgument &A = TA[i];
-  if (A.getKind() != TemplateArgument::Type)
+
+  if (Args[i].getKind() != TemplateArgument::Type)
     return MakeCXType(QualType(), GetTU(CT));
-  return MakeCXType(A.getAsType(), GetTU(CT));
+
+  return MakeCXType(Args[i].getAsType(), GetTU(CT));
 }
 
 unsigned clang_Type_visitFields(CXType PT,