Page MenuHomePhabricator

[libclang] Check for a record declaration before a template specialization.
ClosedPublic

Authored by emilio on Apr 21 2017, 6:15 AM.

Details

Summary

This allows us to see the default template parameters too.

This is a regression in clang 4.0, so uplifting to a dot release would be neat (but not required, I guess).

The regression was caused by https://reviews.llvm.org/D26663.

Fixes bug 32539 (https://bugs.llvm.org//show_bug.cgi?id=32539).

Diff Detail

Repository
rL LLVM

Event Timeline

emilio created this revision.Apr 21 2017, 6:15 AM

Please post the diff with full context (git diff -U9999).

emilio updated this revision to Diff 96130.Apr 21 2017, 6:37 AM

Full diff, thanks @arphaman :)

arphaman added inline comments.Apr 21 2017, 9:14 AM
clang/test/Index/print-type.cpp
118 ↗(On Diff #96130)

Hmm, that's an interesting change. I assume the test passed prior to this patch, right? Did this output change purely because of the change in GetTemplateArguments?

emilio added inline comments.Apr 21 2017, 10:36 AM
clang/test/Index/print-type.cpp
118 ↗(On Diff #96130)

Yup, that's right. This makes the type and the canonical type match in this case. In this case we lose a tiny bit of information (mainly, that the template argument was used through a typedef), but the alternative is to lose information about default template parameters, which seems worse to me (and is a regression, while template argument packs haven't been inspectionable until libclang 4.0).

arphaman accepted this revision.Apr 24 2017, 6:15 AM

I see, thanks for explaining. LGTM. Do you need someone to commit it?

This revision is now accepted and ready to land.Apr 24 2017, 6:15 AM

Yes, please! I've submitted a few patches, but still no commit access.

Thanks!

Sure. You can ask Chris Lattner for commit access for future patches :)

This revision was automatically updated to reflect the committed changes.

@emilio, you can request to merge this into 4.0.1 by using a script called merge-request.sh (http://lists.llvm.org/pipermail/llvm-dev/2017-March/111530.html).

rsmith added a subscriber: rsmith.Apr 26 2017, 2:35 PM

This change looks like it introduces a regression itself: given

template<typename T> struct A {};
template<typename T> using B = A<T*>;
B<int> bi;

... requesting the template arguments for the type B<int> changes from producing int to producing int* with this patch, which seems to directly oppose the intentions of D26663.

If the intent of this libclang function is to request the template arguments of the type as written, this change is wrong. If the intent is to request the template arguments of the desugared type, then D26663 is wrong. But either way, it seems that reversing the order of these checks causes us to produce inconsistent results. Another example of the inconsistency:

template<typename T> using C = T;

With this patch, requesting the template arguments for C<int> gives int but requesting the template arguments of C<A<void>> gives void rather than A<void>.

This change looks like it introduces a regression itself: given

template<typename T> struct A {};
template<typename T> using B = A<T*>;
B<int> bi;

... requesting the template arguments for the type B<int> changes from producing int to producing int* with this patch, which seems to directly oppose the intentions of D26663.

FWIW when I wrote D26663, I did it because arguments weren't inspectionable at all for using declarations. Actually both of them would've worked for me.

template<typename T> using C = T;

With this patch, requesting the template arguments for C<int> gives int but requesting the template arguments of C<A<void>> gives void rather than A<void>.

That being said, that is _specially_ annoying (good catch btw). Annoying enough to justify reverting this change I'd say...

I'll submit something with the test-cases. I guess people that depended on the old translation will just need to live with it... shrug.

I guess I could do something clever like: If it's both a ClassTemplateSpecializationDecl, and a TemplateSpecializationType, and the template arguments of the second happen to not be a template specialization, then use the ClassTemplateSpecializationDecl, otherwise the other, but that seems fairly tricky to maintain, so I'd rather just do the former.

Thanks for looking through it, btw.

FWIW when I wrote D26663, I did it because arguments weren't inspectionable at all for using declarations. Actually both of them would've worked for me.

Also, this bit was not completely accurate, and you could inspect ones, but not others (you could inspect the ones that desugared into a RecordDecl that happened to be a template specialization, as shown by the bug you pointed out).