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@index_specialization@ST>1#T@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@SpecializationDecl>#$@S@Cls | <no-cgname> | Ref,RelCont | rel: 1 // CHECK: [[@LINE-4]]:36 | class/C++ | Cls | c:@S@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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
