Page MenuHomePhabricator

[Objective-C] Miscellaneous -fobjc-weak Fixes

Authored by bkelley on Mar 15 2017, 3:44 PM.



After examining the remaining uses of LangOptions.ObjCAutoRefCount, found a some additional places to also check for ObjCWeak not covered by previous test cases. Added a test file to verify all the code paths that were changed.

Diff Detail

Event Timeline

rjmccall edited edge metadata.Mar 16 2017, 9:40 AM

Same thing about parts of this patch — please try to just check T.hasNonTrivialObjCLifetime() when reasonable.

Thank you for all this, by the way. :)

Thanks for all the feedback! I think things are looking a lot better now.

bkelley updated this revision to Diff 92220.Mar 17 2017, 4:05 PM

Integrated feedback from @rjmccall

rjmccall added inline comments.Mar 20 2017, 11:08 PM
3773 ↗(On Diff #92220)

Is this method not identical in behavior to hasNonTrivialObjCLifetime()?

bkelley updated this revision to Diff 92892.Mar 23 2017, 6:01 PM
bkelley added inline comments.
3773 ↗(On Diff #92220)

Yes, but I didn't see how to extract that information, which is on the internal QualType. But after examining the usage of this function in SemaInit.cpp:6681, I think we need something similar to the isObjCLifetimeType() implementation above.

rjmccall added inline comments.Mar 24 2017, 9:13 AM

Oh, I see what's happening here. The special case for ARC is actually wholly unnecessary. The code's been written as if MTE->getType() for an ARC temporary will just have type "id". In fact, it'll have a lifetime-qualified type, dependent on the type of the reference which has been bound to it. So we need to mark that there's a temporary cleanup if the type has non-trivial lifetime, i.e. if MTE->getType().hasNonTrivialObjCLifetime(); but this is already considered as part of MTE->getType().isDestructedType(), and the storage-duration condition there is correct as well. So really we should just delete this entire clause.

That should eliminate the need for Type::isNonTrivialObjCLifetimeType(). In general, we don't want methods like that to ever exist: lifetime is determined by qualifiers, and querying a Type instead of a QualType has the potential to implicitly remove qualifiers.

bkelley updated this revision to Diff 92966.Mar 24 2017, 9:38 AM
bkelley marked an inline comment as done.

Updated with latest feedback from @rjmccall


Thanks for looking in to this and for the explanation. I've removed the check and the changes to Type.

rjmccall accepted this revision.Mar 24 2017, 12:21 PM

LGTM, thanks!

This revision is now accepted and ready to land.Mar 24 2017, 12:21 PM

Thank you @rjmccall for the approval. I don't have commit access; would someone be willing to commit this path for me please? Thanks!

bkelley closed this revision.Mar 29 2017, 11:28 AM