Page MenuHomePhabricator

libclang: Restore the CXXRecordDecl path for clang_Type_getNumTemplateArguments and clang_Type_getTemplateArgumentAsType
ClosedPublic

Authored by emilio on Nov 21 2016, 3:15 AM.

Details

Summary

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

Diff Detail

Repository
rL LLVM

Event Timeline

emilio updated this revision to Diff 78697.Nov 21 2016, 3:15 AM
emilio retitled this revision from to libclang: Restore the CXXRecordDecl path for clang_Type_getNumTemplateArguments and clang_Type_getTemplateArgumentAsType.
emilio updated this object.
emilio added a reviewer: akyrtzi.
emilio set the repository for this revision to rL LLVM.
emilio added a project: Restricted Project.
emilio updated this revision to Diff 79251.Nov 24 2016, 12:18 PM
emilio edited edge metadata.

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

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 :)

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

akyrtzi edited edge metadata.Dec 1 2016, 4:02 PM

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.

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 updated this revision to Diff 80092.Dec 2 2016, 10:25 AM
emilio edited edge metadata.

Now with proper tests :)

EricWF resigned from this revision.Dec 3 2016, 8:24 AM
EricWF removed a reviewer: EricWF.

Sorry a bad Herald regex added me as a reviewer.

skalinichev added a subscriber: skalinichev.

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

skalinichev edited edge metadata.Dec 4 2016, 1:49 AM
skalinichev added a subscriber: compnerd.

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

skalinichev added inline comments.Dec 4 2016, 2:03 AM
test/Index/keep-going.cpp
22

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 added a comment.Dec 4 2016, 2:16 AM

Thanks for the comments!

Will address them ASAP and post back.

test/Index/keep-going.cpp
22

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 added inline comments.Dec 4 2016, 2:17 AM
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 this revision to Diff 80205.Dec 4 2016, 2:57 AM
emilio edited edge metadata.

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 added inline comments.Dec 4 2016, 3:52 AM
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 this revision to Diff 80217.Dec 4 2016, 3:50 PM

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 added inline comments.Dec 10 2016, 5:42 AM
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.

959–982

static

962

Use llvm::Optional instead

991

Ty -> QT

emilio updated this revision to Diff 81187.Dec 12 2016, 11:11 PM

Thanks for the review, good catches :)

Updated again.

skalinichev added inline comments.Dec 13 2016, 9:52 AM
tools/libclang/CXType.cpp
962

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 added inline comments.Dec 13 2016, 9:58 AM
tools/libclang/CXType.cpp
962

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 updated this revision to Diff 81251.Dec 13 2016, 10:14 AM

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

emilio updated this revision to Diff 81325.Dec 13 2016, 4:24 PM

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 marked 17 inline comments as done.Dec 14 2016, 12:15 AM

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

akyrtzi accepted this revision.Dec 16 2016, 1:51 PM
akyrtzi edited edge metadata.

Committed via r289995, thanks!

This revision is now accepted and ready to land.Dec 16 2016, 1:51 PM
akyrtzi closed this revision.Dec 16 2016, 1:51 PM