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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
