Page MenuHomePhabricator

ObjC: Use a new type for ObjC type parameter (patch 2 out of 3)
ClosedPublic

Authored by manmanren on Aug 2 2016, 12:47 PM.

Details

Summary

This depends on https://reviews.llvm.org/D23078

ObjC generics: Add ObjCTypeParamType in the type system.

We also need to add ObjCTypeParamTypeLoc. ObjCTypeParamType supports the
representation of "T <protocol>" where T is a type parameter. Before this,
we use TypedefType to represent the type parameter for ObjC.

ObjCTypeParamType has "ObjCTypeParamDecl *OTPDecl" and it extends from
ObjCProtocolQualifiers.

The last patch (patch #3) will start using this new type.

Diff Detail

Repository
rL LLVM

Event Timeline

manmanren updated this revision to Diff 66527.Aug 2 2016, 12:47 PM
manmanren retitled this revision from to ObjC: Use a new type for ObjC type parameter (patch 2 out of 3).
manmanren updated this object.
manmanren added a reviewer: doug.gregor.
manmanren added a subscriber: cfe-commits.
doug.gregor requested changes to this revision.Aug 19 2016, 3:50 PM
doug.gregor edited edge metadata.

A couple of comments above, but this is looking very good.

include/clang/AST/RecursiveASTVisitor.h
1037 ↗(On Diff #66527)

I'm sorta shocked that we don't visit the protocol qualifiers here, but I guess we haven't been doing that for ObjCObjectType all along. Weird.

1270 ↗(On Diff #66527)

Same shock at not visiting the protocol qualifiers here :)

include/clang/AST/Type.h
4786 ↗(On Diff #66527)

This is an interesting choice. Objective-C type parameters were treated like typedefs before, so they always act like their underlying type (e.g., because they are typedef name declarations). Why isn't ObjCTypeParamType sugar for the underlying type of the type parameter (I.e., the bound) w/ the protocol qualifiers?

include/clang/AST/TypeLoc.h
696 ↗(On Diff #66527)

The angle bracket locations don't exist if you don't have any protocols, so you could conceivably put them into the extra local data.

include/clang/Serialization/ASTBitCodes.h
906 ↗(On Diff #66527)

Comment please, even if it's trivial.

lib/AST/ItaniumMangle.cpp
2916 ↗(On Diff #66527)

This doesn't look right. Typedef names aren't mangled; we should mangle based on building the ObjCPointerType with the canonical type of T->getDecl()->getUnderlyingType() w/ the protocol qualifiers added to it.

That said, I don't think this case will ever come up, because you can't define something that would C++ mangling within an Objective-C @interface.

lib/AST/MicrosoftMangle.cpp
2318 ↗(On Diff #66527)

I'd expect this to have the same implementation as the Itanium one: the canonical type w/ the underlying type + protocol qualifiers. Again, I don't think this code path can ever get triggered.

This revision now requires changes to proceed.Aug 19 2016, 3:50 PM

Thanks for reviewing!

Manman

include/clang/AST/RecursiveASTVisitor.h
1037 ↗(On Diff #66527)

Right below, we don't visit the protocol qualifiers for ObjCObjectType :]
If you think we should, I can patch it up for both types.

1270 ↗(On Diff #66527)

Same here.

include/clang/AST/Type.h
4786 ↗(On Diff #66527)

Are you suggesting to canonicalize ObjCTypeParamType to the underlying type with the protocol qualifiers as well? Or just desugaring?

I think I have tried to set ObjCTypeParamType as NON_CANONICAL_TYPE in TypeNodes.def, but had some issues.

doug.gregor added inline comments.Aug 23 2016, 3:46 PM
include/clang/AST/RecursiveASTVisitor.h
1037 ↗(On Diff #66527)

Yes, I think we should.

include/clang/AST/Type.h
4786 ↗(On Diff #66527)

Canonicalize to the underlying type + protocol qualifiers. We don't need the desugaring part.

manmanren updated this revision to Diff 69764.Aug 30 2016, 3:13 PM
manmanren edited edge metadata.

Address Doug's comment. ObjCTypeParamType is a non-canonical type now, it is canonicalized to the underlying type with protocol qualifiers.

Hi Doug,

Can you take a look at the updated version?

Thanks,
Manman

doug.gregor accepted this revision.Sep 12 2016, 4:05 PM
doug.gregor edited edge metadata.

This looks great, thank you!

This revision is now accepted and ready to land.Sep 12 2016, 4:05 PM
This revision was automatically updated to reflect the committed changes.