[PATCH] D74830: [clang][Index] Fix the incomplete instantiations in libindex.

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet added inline comments.


================
Comment at: clang/lib/Index/IndexingContext.cpp:217
+    // Fallback to primary template if this is an incomplete specialization.
+    if (SD->getTemplateSpecializationKind() == TSK_Undeclared)
+      return SD->getSpecializedTemplate()->getTemplatedDecl();
----------------
kadircet wrote:
> maybe return templateddecl iff instantiationpattern is null ?
nit:

```
if(const auto *Temp ...)
 return Temp..;
return templateddecl
```


================
Comment at: clang/lib/Index/IndexingContext.cpp:173
+     // Incomplete template specialization is not instantiated, we still treat
+     // it as an instantiation as we'd like to keep the canonicalized result
+     // consistent.
----------------
not just instantiation but rather **implicit** instantiation.


================
Comment at: clang/lib/Index/IndexingContext.cpp:216
+      return Template;
+    // Fallback to primary template if this is an incomplete specialization.
+    return SD->getSpecializedTemplate()->getTemplatedDecl();
----------------
there might be other reasons for not having an instantiation yet (e.g dependent code), maybe just say fallback to class template if no instantiation is available yet?


================
Comment at: clang/test/Index/Core/index-instantiated-source.cpp:103
+// explicit instantiations.
+template class Foo<float>;
+Foo<float> t3;
----------------
yes this is an explicit instantiation and will have TSKKind set properly.

but i was talking about an explicit specialization in the example above, i.e:

```
template <> class Foo<float>;
```

which will still have `TSK_Undeclared` (I believe, haven't checked).


and let me make myself clear, I am not opposed to the idea of making this also a ref to the primary template instead of the specialization, I just want to make sure we spell it out in the comments and tests explicitly.


================
Comment at: clang/test/Index/Core/index-instantiated-source.cpp:104
+template class Foo<float>;
+Foo<float> t3;
+// CHECK: [[@LINE-1]]:1 | class(Gen)/C++ | Foo | c:@N at index_specialization@ST>1#T at Foo | <no-cgname> | Ref,RelCont | rel: 1
----------------
this is not valid, as you can't define a variable of incomplete type, this needs to be in a function declaration (as I provided in the example) or you can also try making this a pointer as you did below.

i.e.:

```
void foo(Foo<float>);
```


================
Comment at: clang/test/Index/Core/index-source.cpp:324
 // CHECK:   RelSpecialization | functionSp | c:@FT@>2#T#NIfunctionSp#v#
-// CHECK: [[@LINE-3]]:17 | class(Gen,TS)/C++ | SpecializationDecl | c:@S at SpecializationDecl>#$@S at Cls | <no-cgname> | Ref,RelCont | rel: 1
 // CHECK: [[@LINE-4]]:36 | class/C++ | Cls | c:@S at Cls | <no-cgname> | Ref,RelCont | rel: 1
----------------
hokein wrote:
> kadircet wrote:
> > this looks like a regression, previously when indexing this symbol it was known that first template argument was a `SpecializationDecl<Cls>` now it is class template instead.
> > 
> > (same for the 2 below)
> yeah, that was my thought previously, but it turns out not -- it is a bug I think, there is no explicit specialization for `SpecializationDecl<Cls>`, so we should give the primary template. 
> 
> Take a look on Line 60 of this file, `TemplCls<int> gtv(0);` it gives the primary class template as well. 
yeah makes sense actually, also this is a Ref, not a decl or def so it is not very logical for it to point at some implicit decl.

SGTM


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D74830/new/

https://reviews.llvm.org/D74830