[PATCH] D23079: ObjC: Use a new type for ObjC type parameter (patch 2 out of 3)

[Doug Gregor via cfe-commits]

doug.gregor requested changes to this revision.
doug.gregor added a comment.
This revision now requires changes to proceed.

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


================
Comment at: include/clang/AST/RecursiveASTVisitor.h:1037
@@ -1036,1 +1036,3 @@
 
+DEF_TRAVERSE_TYPE(ObjCTypeParamType, {})
+
----------------
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.

================
Comment at: include/clang/AST/RecursiveASTVisitor.h:1270
@@ -1267,1 +1269,3 @@
 
+DEF_TRAVERSE_TYPELOC(ObjCTypeParamType, {})
+
----------------
Same shock at not visiting the protocol qualifiers here :)

================
Comment at: include/clang/AST/Type.h:4786
@@ +4785,3 @@
+  bool isSugared() const { return false; }
+  QualType desugar() const { return QualType(this, 0); }
+
----------------
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?

================
Comment at: include/clang/AST/TypeLoc.h:696
@@ -695,1 +695,3 @@
 
+struct ObjCTypeParamTypeLocInfo {
+  SourceLocation ProtocolLAngleLoc;
----------------
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.

================
Comment at: include/clang/Serialization/ASTBitCodes.h:906
@@ -906,1 +905,3 @@
+      TYPE_PIPE                  = 43,
+      TYPE_OBJC_TYPE_PARAM       = 44
     };
----------------
Comment please, even if it's trivial.

================
Comment at: lib/AST/ItaniumMangle.cpp:2916
@@ +2915,3 @@
+  }
+  mangleSourceName(T->getDecl()->getIdentifier());
+}
----------------
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.

================
Comment at: lib/AST/MicrosoftMangle.cpp:2318
@@ +2317,3 @@
+                                         SourceRange Range) {
+  mangleType(T->getDecl()->getUnderlyingType(), Range);
+}
----------------
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.


https://reviews.llvm.org/D23079