[cfe-commits] r75314 - in /cfe/trunk: include/clang/AST/ include/clang/Analysis/PathSensitive/ lib/AST/ lib/Analysis/ lib/CodeGen/ lib/Frontend/ lib/Sema/ test/CodeGenObjC/ test/PCH/ test/SemaObjC/ test/SemaObjCXX/
steve naroff
snaroff at apple.com
Mon Jul 13 09:14:57 PDT 2009
On Jul 13, 2009, at 2:00 AM, Chris Lattner wrote:
> On Jul 10, 2009, at 4:35 PM, Steve Naroff wrote:
>> Author: snaroff
>> Date: Fri Jul 10 18:34:53 2009
>> New Revision: 75314
>>
>> URL: http://llvm.org/viewvc/llvm-project?rev=75314&view=rev
>> Log:
>> This patch includes a conceptually simple, but very intrusive/
>> pervasive change.
>
> Yay, thanks for working on this.
No problem. Thanks a lot for taking your time to comment on a patch of
this girth...much appreciated.
> Does this add support for Class<x> and friends? If so, please add
> testcases.
>
It provides almost all of the support. I wanted to finish adding
"Class<x>" in a separate patch. When I do, I'll add test cases.
> ...
>
>> class ObjCObjectPointerType : public Type, public
>> llvm::FoldingSetNode {
>> QualType PointeeType; // will always point to an interface type.
>>
>> // List of protocols for this protocol conforming object type
>> // List is sorted on protocol name. No protocol is entered more
>> than once.
>> llvm::SmallVector<ObjCProtocolDecl*, 8> Protocols;
>>
>> ObjCObjectPointerType(QualType T, ObjCProtocolDecl **Protos,
>> unsigned NumP) :
>> Type(ObjCObjectPointer, QualType(), /*Dependent=*/false),
>> PointeeType(T), Protocols(Protos, Protos+NumP) { }
>> friend class ASTContext; // ASTContext creates these.
>> friend class ObjCInterfaceType; // To enable 'id' and 'Class'
>> predicates.
>>
>> static ObjCInterfaceType *IdInterfaceT;
>> static ObjCInterfaceType *ClassInterfaceT;
>> static void setIdInterface(QualType T) {
>
> Please don't use static members within the class. This is not safe
> when there are multiple translation units open at a time and when
> threading is happening. Please put state like this in ASTContext.
>
If I put the state in ASTContext, then
ObjCObjectPointerType::isObjCIdType() and Type::isObjCIdType() will
need a pointer to it (which is what I was trying to avoid).
For now, I'll just make the change and live with a less convenient
API. We could have a low cost union (by playing games with the low
order bits of the ObjCInterfaceType), however I'd prefer not to fiddle
with this for now.
>> @@ -3198,8 +3186,30 @@
>> /// compatible for assignment from RHS to LHS. This handles
>> validation of any
>> /// protocol qualifiers on the LHS or RHS.
>> ///
>> +/// FIXME: Move the following to ObjCObjectPointerType/
>> ObjCInterfaceType.
>> +bool ASTContext::canAssignObjCInterfaces(const
>> ObjCObjectPointerType *LHSOPT,
>> + const
>> ObjCObjectPointerType *RHSOPT) {
>> + // If either interface represents the built-in 'id' or 'Class'
>> types,
>> + // then return true (no need to call canAssignObjCInterfaces()).
>> + if (LHSOPT->isObjCIdType() || RHSOPT->isObjCIdType() ||
>> + LHSOPT->isObjCClassType() || RHSOPT->isObjCClassType())
>> + return true;
>
> It seems common to check for "isObjCIdType || isObjCClassType" and:
>
>> bool ASTContext::canAssignObjCInterfaces(const ObjCInterfaceType
>> *LHS,
>> const ObjCInterfaceType
>> *RHS) {
>> + // If either interface represents the built-in 'id' or 'Class'
>> types,
>> + // then return true.
>> + if (LHS->isObjCIdInterface() || RHS->isObjCIdInterface() ||
>> + LHS->isObjCClassInterface() || RHS->isObjCClassInterface())
>
>
> "isObjCIdInterface || isObjCClassInterface"
>
> Does it make sense to add direct predicates for these?
Sure. How about isObjCBuiltinType() and isObjCBuiltinInterface()?
>
>
>> +++ cfe/trunk/lib/AST/ExprConstant.cpp Fri Jul 10 18:34:53 2009
>> @@ -382,7 +382,8 @@
>> const Expr* SubExpr = E->getSubExpr();
>>
>> // Check for pointer->pointer cast
>> - if (SubExpr->getType()->isPointerType()) {
>> + if (SubExpr->getType()->isPointerType() ||
>> + SubExpr->getType()->isObjCObjectPointerType()) {
>
> Would it make sense to add a "isAnyPointerType()" method to do both
> these checks?
I don't think so. It will save a few keystrokes, however I believe the
above is clearer.
>
>
>> +++ cfe/trunk/lib/Analysis/BasicObjCFoundationChecks.cpp Fri Jul 10
>> 18:34:53 2009
>
> I'll let Ted review libanalysis and friends.
>
Ted's already reviewed these (since he contributed many of libanalysis
changes...).
>
>> +++ cfe/trunk/lib/CodeGen/CGDebugInfo.cpp Fri Jul 10 18:34:53 2009
>> @@ -761,8 +761,10 @@
>> // Unsupported types
>> return llvm::DIType();
>> case Type::ObjCObjectPointer: // Encode id<p> in debug info just
>> like id.
>> - return Slot = getOrCreateType(M->getContext().getObjCIdType(),
>> Unit);
>> -
>> + {
>> + ObjCObjectPointerType *OPT = cast<ObjCObjectPointerType>(Ty);
>> + return Slot = CreateType(OPT->getInterfaceType(), Unit);
>> + }
>
> I don't think this is right. Given something like id<foo>, this
> should call CreateType on "id". Given NSString<foo> it should call
> CreateType on "NSString*", not "NSString".
>
I'll take a look.
>> +++ cfe/trunk/lib/CodeGen/CGExprScalar.cpp Fri Jul 10 18:34:53 2009
>> @@ -987,7 +987,7 @@
>> }
>>
>> Value *ScalarExprEmitter::EmitAdd(const BinOpInfo &Ops) {
>> - if (!Ops.Ty->isPointerType()) {
>> + if (!Ops.Ty->isPointerType() && !Ops.Ty-
>> >isObjCObjectPointerType()) {
>
> Another candidate for "isAnyPointerType"? What happens when you add
> an objc pointer to an integer?
You can't have an objc pointer to an integer (that's why I called the
class ObjCObjectPointerType)....an ObjCObjectPointerType can only
refer to an interface (built-in or user-defined).
Maybe I don't understand your question.
> Isn't it the same as adding to a pointer to struct? It seems that
> this code could be merged now.
>
>> +++ cfe/trunk/lib/CodeGen/CodeGenTypes.cpp Fri Jul 10 18:34:53 2009
>> @@ -215,6 +215,7 @@
>> const llvm::Type *CodeGenTypes::ConvertNewType(QualType T) {
>> const clang::Type &Ty = *Context.getCanonicalType(T);
>>
>> + //T->dump();
>
> Please remove.
Sure.
>
>
>> switch (Ty.getTypeClass()) {
>> #define TYPE(Class, Base)
>> #define ABSTRACT_TYPE(Class, Base)
>> @@ -353,10 +354,14 @@
>> return T;
>> }
>>
>> - case Type::ObjCObjectPointer:
>> - // Protocols don't influence the LLVM type.
>> - return ConvertTypeRecursive(Context.getObjCIdType());
>> -
>> + case Type::ObjCObjectPointer: {
>> + // Qualified id types don't influence the LLVM type, here we
>> always return
>> + // an opaque type for 'id'.
>> + const llvm::Type *&T = InterfaceTypes[0];
>> + if (!T)
>> + T = llvm::OpaqueType::get();
>> + return llvm::PointerType::getUnqual(T);
>> + }
>
> This doesn't seem like the right thing, can you check with Daniel on
> this?
Daniel suggested this solution to me (so I guess I've already checked:-)
>
>
>> +++ cfe/trunk/lib/Sema/SemaDecl.cpp Fri Jul 10 18:34:53 2009
>> @@ -527,13 +525,15 @@
>> case 2:
>> if (!TypeID->isStr("id"))
>> break;
>> - Context.setObjCIdType(Context.getTypeDeclType(New));
>> - objc_types = true;
>> + // Install the built-in type for 'id', ignoring the current
>> definition.
>> + New->setTypeForDecl(Context.getObjCIdType().getTypePtr());
>> + return;
>> break;
>
> Please remove the dead break.
Will do.
>
>
>> +++ cfe/trunk/lib/Sema/SemaExpr.cpp Fri Jul 10 18:34:53 2009
>> @@ -1844,6 +1844,17 @@
>> BaseExpr = RHSExp;
>> IndexExpr = LHSExp;
>> ResultType = PTy->getPointeeType();
>> + } else if (const ObjCObjectPointerType *PTy =
>> + LHSTy->getAsObjCObjectPointerType()) {
>> + BaseExpr = LHSExp;
>> + IndexExpr = RHSExp;
>> + ResultType = PTy->getPointeeType();
>> + } else if (const ObjCObjectPointerType *PTy =
>> + RHSTy->getAsObjCObjectPointerType()) {
>> + // Handle the uncommon case of "123[Ptr]".
>> + BaseExpr = RHSExp;
>> + IndexExpr = LHSExp;
>> + ResultType = PTy->getPointeeType();
>
> It seems that the objc and normal pointer type can be merged with
> your new "getpointeetype" method.
>
Makes sense. Note: This is an example of a cleanup that I wanted to
defer (for this mega patch).
>> @@ -2212,12 +2225,71 @@
>> << DeclarationName(&Member) << int(OpKind == tok::arrow));
>> }
>>
>> + // Handle properties on ObjC 'Class' types.
>> + if (OpKind == tok::period && (BaseType->isObjCClassType())) {
>
> It's minor, but no need for the redundant parens here.
>
O.k.
>
>> + // Also must look for a getter name which uses property syntax.
>> + Selector Sel =
>> PP.getSelectorTable().getNullarySelector(&Member);
>> + if (ObjCMethodDecl *MD = getCurMethodDecl()) {
>> + ObjCInterfaceDecl *IFace = MD->getClassInterface();
>> + ObjCMethodDecl *Getter;
>> + // FIXME: need to also look locally in the implementation.
>> + if ((Getter = IFace->lookupClassMethod(Sel))) {
>> + // Check the use of this method.
>> + if (DiagnoseUseOfDecl(Getter, MemberLoc))
>> + return ExprError();
>> + }
>> + // If we found a getter then this may be a valid dot-
>> reference, we
>> + // will look for the matching setter, in case it is needed.
>> + Selector SetterSel =
>> + SelectorTable::constructSetterName(PP.getIdentifierTable(),
>> + PP.getSelectorTable(),
>> &Member);
>
>
>> + ObjCMethodDecl *Setter = IFace->lookupClassMethod(SetterSel);
>> + if (!Setter) {
>> + // If this reference is in an @implementation, also check
>> for 'private'
>> + // methods.
>> + Setter = FindMethodInNestedImplementations(IFace,
>> SetterSel);
>> + }
>> + // Look through local category implementations associated
>> with the class.
>> + if (!Setter) {
>> + for (unsigned i = 0; i < ObjCCategoryImpls.size() && !
>> Setter; i++) {
>> + if (ObjCCategoryImpls[i]->getClassInterface() == IFace)
>> + Setter = ObjCCategoryImpls[i]-
>> >getClassMethod(SetterSel);
>> + }
>> + }
>
> This code looks like it is something that should be factored out
> into a helper method. Are there other pieces of code doing the same
> lookup algorithm?
>
Yep. I agree a helper would be nice. Since this doesn't really relate
to the ObjCObjectPointerType related changes, I'll note this as a
separate cleanup (the only reason in showed up in the patch is I move
the code).
>> // Handle properties on qualified "id" protocols.
>> + const ObjCObjectPointerType *QIdTy;
>> + if (OpKind == tok::period && (QIdTy = BaseType-
>> >getAsObjCQualifiedIdType())) {
>
> Can this be merged with the non-qualified case?
Possibly...will need to look deeper.
>
>
>> @@ -3517,6 +3523,29 @@
>> return Incompatible;
>> }
>>
>> + if (isa<ObjCObjectPointerType>(lhsType)) {
>> + if (rhsType->isIntegerType())
>> + return IntToPointer;
>> +
>> + if (isa<PointerType>(rhsType)) {
>> + QualType lhptee = lhsType->getAsObjCObjectPointerType()-
>> >getPointeeType();
>> + QualType rhptee = rhsType->getAsPointerType()-
>> >getPointeeType();
>> + return CheckPointeeTypesForAssignment(lhptee, rhptee);
>
> Why not use lhsType->getPointeeType() (and also for rhsType)?
Since the routine is already doing the "isa" sniffing to determine
what types we have, I didn't think it would be any clearer (or
efficient).
>
>
>> + }
>> + if (rhsType->isObjCObjectPointerType()) {
>> + QualType lhptee = lhsType->getAsObjCObjectPointerType()-
>> >getPointeeType();
>> + QualType rhptee = rhsType->getAsObjCObjectPointerType()-
>> >getPointeeType();
>> + return CheckPointeeTypesForAssignment(lhptee, rhptee);
>
> That would allow merging this case in as well.
Same response as above.
>
>
>> @@ -3776,12 +3823,23 @@
>>
>> // Put any potential pointer into PExp
>> Expr* PExp = lex, *IExp = rex;
>> - if (IExp->getType()->isPointerType())
>> + if (IExp->getType()->isPointerType() ||
>> + IExp->getType()->isObjCObjectPointerType())
>
> ->isAnyPointerType()
As a said earlier, I'm not convinced adding another predicate would
make the code clearer (though it would definitely make it more terse).
Maybe a better name would sway my opinion. isCOrObjCPointer() is more
descriptive, however one of the uglier names I've ever seen:-)
>
>
>> std::swap(PExp, IExp);
>>
>> - if (const PointerType *PTy = PExp->getType()-
>> >getAsPointerType()) {
>> + if (PExp->getType()->isPointerType() ||
>> + PExp->getType()->isObjCObjectPointerType()) {
>
> again.
>
>> +
>> if (IExp->getType()->isIntegerType()) {
>> - QualType PointeeTy = PTy->getPointeeType();
>> + QualType PointeeTy;
>> + const PointerType *PTy;
>> + const ObjCObjectPointerType *OPT;
>> +
>> + if ((PTy = PExp->getType()->getAsPointerType()))
>> + PointeeTy = PTy->getPointeeType();
>> + else if ((OPT = PExp->getType()-
>> >getAsObjCObjectPointerType()))
>> + PointeeTy = OPT->getPointeeType();
>
> This should be able to use QualType::getPointeeType() instead of the
> if/elseif
This isn't possible without more code reorganization (I already tried
it). The code that follows is interested in both PTy and OPT (not only
the pointee type).
>
>
>> @@ -3855,10 +3913,16 @@
>> if (CompLHSTy) *CompLHSTy = compType;
>> return compType;
>> }
>> -
>> +
>> // Either ptr - int or ptr - ptr.
>> - if (const PointerType *LHSPTy = lex->getType()-
>> >getAsPointerType()) {
>> - QualType lpointee = LHSPTy->getPointeeType();
>> + if (lex->getType()->isPointerType() ||
>> + lex->getType()->isObjCObjectPointerType()) {
>
> isAnyPointerType()
You really like that predicate:-)
>
>
>> + QualType lpointee;
>> + if (const PointerType *LHSPTy = lex->getType()-
>> >getAsPointerType())
>> + lpointee = LHSPTy->getPointeeType();
>> + else if (const ObjCObjectPointerType *OPT =
>> + lex->getType()->getAsObjCObjectPointerType())
>> + lpointee = OPT->getPointeeType();
>
> lex->getType()->getPointeeType()
This one does look like a candidate (good catch).
>
>
>> @@ -4226,19 +4289,27 @@
>> ImpCastExprToType(rex, lType);
>> return ResultTy;
>> }
>> + if (lType->isObjCObjectPointerType() && rType-
>> >isObjCObjectPointerType()) {
>> + if (!Context.areComparableObjCPointerTypes(lType, rType)) {
>> + Diag(Loc,
>> diag::ext_typecheck_comparison_of_distinct_pointers)
>> << lType << rType << lex->getSourceRange() << rex-
>> >getSourceRange();
>> }
>> + if (lType->isObjCQualifiedIdType() && rType-
>> >isObjCQualifiedIdType()) {
>> + if (ObjCQualifiedIdTypesAreCompatible(lType, rType, true)) {
>> + ImpCastExprToType(rex, lType);
>> + return ResultTy;
>> + } else {
>> + Diag(Loc, diag::warn_incompatible_qualified_id_operands)
>> + << lType << rType << lex->getSourceRange() << rex-
>> >getSourceRange();
>> + ImpCastExprToType(rex, lType);
>> + return ResultTy;
>
> This else case can be unnested and can fall through to share the
> ImpCastExprToType + return.
>
>> + }
>> + }
>> + ImpCastExprToType(rex, lType);
>> + return ResultTy;
>> }
>> }
>
>> - if ((lType->isPointerType() || lType->isObjCQualifiedIdType()) &&
>> + if ((lType->isPointerType() || lType->isObjCObjectPointerType())
>> &&
>
> isAnyPointerType()
>
>> @@ -4250,7 +4321,7 @@
>> return ResultTy;
>> }
>> if (lType->isIntegerType() &&
>> - (rType->isPointerType() || rType->isObjCQualifiedIdType())) {
>> + (rType->isPointerType() || rType-
>> >isObjCObjectPointerType())) {
>
> isAnyPointerType()
>
>> @@ -4524,9 +4594,17 @@
>> Diag(OpLoc, diag::warn_increment_bool) << Op->getSourceRange();
>> } else if (ResType->isRealType()) {
>> // OK!
>> - } else if (const PointerType *PT = ResType->getAsPointerType()) {
>> + } else if (ResType->getAsPointerType() ||ResType-
>> >isObjCObjectPointerType()) {
>
> isAnyPointerType(), please don't use "getAs" in a bool context.
>
>> + QualType PointeeTy;
>> +
>> + if (const PointerType *PTy = ResType->getAsPointerType())
>> + PointeeTy = PTy->getPointeeType();
>> + else if (const ObjCObjectPointerType *OPT =
>> + ResType->getAsObjCObjectPointerType())
>> + PointeeTy = OPT->getPointeeType();
>
> QualType::getPointeeType().
>
>> +++ cfe/trunk/lib/Sema/SemaExprCXX.cpp Fri Jul 10 18:34:53 2009
>> @@ -1482,7 +1482,8 @@
>> QualType Sema::FindCompositePointerType(Expr *&E1, Expr *&E2) {
>> assert(getLangOptions().CPlusPlus && "This function assumes C++");
>> QualType T1 = E1->getType(), T2 = E2->getType();
>> - if(!T1->isPointerType() && !T2->isPointerType())
>> + if(!T1->isPointerType() && !T2->isPointerType() &&
>> + !T1->isObjCObjectPointerType() && !T2-
>> >isObjCObjectPointerType())
>
> isAnyPointerType()
>
>> +++ cfe/trunk/lib/Sema/SemaExprObjC.cpp Fri Jul 10 18:34:53 2009
>
> wow, nice cleanups!
>
>>
>> + if (rhsOPT->qual_empty()) {
>> + // If the RHS is a unqualified interface pointer "NSString*",
>> + // make sure we check the class hierarchy.
>> + if (ObjCInterfaceDecl *rhsID = rhsOPT->getInterfaceDecl()) {
>> + for (ObjCObjectPointerType::qual_iterator I = lhsQID-
>> >qual_begin(),
>> + E = lhsQID->qual_end(); I != E; ++I) {
>> + // when comparing an id<P> on lhs with a static type on
>> rhs,
>> + // see if static class implements all of id's protocols,
>> directly or
>> + // through its super class and categories.
>> + if (!ClassImplementsProtocol(*I, rhsID, true))
>> + return false;
>> + }
>> + return true;
>
> This return can be removed.
O.K.
>
>
>> + }
>> + // If there are no qualifiers and no interface, we have an
>> 'id'.
>> + return true;
>> + }
>> + // Both the right and left sides have qualifiers.
>> for (ObjCObjectPointerType::qual_iterator I = lhsQID-
>> >qual_begin(),
>> E = lhsQID->qual_end(); I != E; ++I) {
>> ObjCProtocolDecl *lhsProto = *I;
>> @@ -803,28 +779,26 @@
>> // when comparing an id<P> on lhs with a static type on rhs,
>> // see if static class implements all of id's protocols,
>> directly or
>> // through its super class and categories.
>> + for (ObjCObjectPointerType::qual_iterator J = rhsOPT-
>> >qual_begin(),
>> + E = rhsOPT->qual_end(); J != E; ++J) {
>> + ObjCProtocolDecl *rhsProto = *J;
>> if (ProtocolCompatibleWithProtocol(lhsProto, rhsProto) ||
>> (compare && ProtocolCompatibleWithProtocol(rhsProto,
>> lhsProto))) {
>> match = true;
>> break;
>> }
>> }
>> + // If the RHS is a qualified interface pointer "NSString<P>*",
>> + // make sure we check the class hierarchy.
>> + if (ObjCInterfaceDecl *rhsID = rhsOPT->getInterfaceDecl()) {
>> + for (ObjCObjectPointerType::qual_iterator I = lhsQID-
>> >qual_begin(),
>> + E = lhsQID->qual_end(); I != E; ++I) {
>> + // when comparing an id<P> on lhs with a static type on
>> rhs,
>> + // see if static class implements all of id's protocols,
>> directly or
>> + // through its super class and categories.
>> + if (ClassImplementsProtocol(*I, rhsID, true)) {
>> + match = true;
>> + break;
>> }
>> }
>
> Does this loop exist elsewhere? If so, please factor out to a
> helper function.
I'll look...
>
>
>> }
>> @@ -837,7 +811,52 @@
>>
>> const ObjCObjectPointerType *rhsQID = rhs-
>> >getAsObjCQualifiedIdType();
>> assert(rhsQID && "One of the LHS/RHS should be id<x>");
>> -
>> +
>> + if (const ObjCObjectPointerType *lhsOPT =
>> + lhs->getAsObjCInterfacePointerType()) {
>> + if (lhsOPT->qual_empty()) {
>> + bool match = false;
>> + if (ObjCInterfaceDecl *lhsID = lhsOPT->getInterfaceDecl()) {
>> + for (ObjCObjectPointerType::qual_iterator I = rhsQID-
>> >qual_begin(),
>> + E = rhsQID->qual_end(); I != E; ++I) {
>> + // when comparing an id<P> on lhs with a static type on
>> rhs,
>> + // see if static class implements all of id's protocols,
>> directly or
>> + // through its super class and categories.
>> + if (ClassImplementsProtocol(*I, lhsID, true)) {
>> + match = true;
>> + break;
>> + }
>> + }
>> + if (!match)
>> + return false;
>
> Please factor the inner loop out to a helper function. This allows
> you to use early return instead of the 'match' bool + break.
>
>> + }
>> + return true;
>> + }
>> + // Both the right and left sides have qualifiers.
>> + for (ObjCObjectPointerType::qual_iterator I = lhsOPT-
>> >qual_begin(),
>> + E = lhsOPT->qual_end(); I != E; ++I) {
>> + ObjCProtocolDecl *lhsProto = *I;
>> + bool match = false;
>> +
>> + // when comparing an id<P> on lhs with a static type on rhs,
>> + // see if static class implements all of id's protocols,
>> directly or
>> + // through its super class and categories.
>> + for (ObjCObjectPointerType::qual_iterator J = rhsQID-
>> >qual_begin(),
>> + E = rhsQID->qual_end(); J != E; ++J) {
>> + ObjCProtocolDecl *rhsProto = *J;
>> + if (ProtocolCompatibleWithProtocol(lhsProto, rhsProto) ||
>> + (compare && ProtocolCompatibleWithProtocol(rhsProto,
>> lhsProto))) {
>> + match = true;
>> + break;
>
> This seems very similar to the above loops, it would be nice to
> commonize them somehow.
>
>> +++ cfe/trunk/lib/Sema/SemaOverload.cpp Fri Jul 10 18:34:53 2009
>
> Please ask Doug to review this.
Sure...Doug and I already
>
>
>> + // Beyond this point, both types need to be C pointers or block
>> pointers.
>> QualType ToPointeeType;
>> - const PointerType* ToTypePtr = ToType->getAsPointerType();
>> - if (ToTypePtr)
>> - ToPointeeType = ToTypePtr->getPointeeType();
>> + if (const PointerType *ToCPtr = ToType->getAsPointerType())
>> + ToPointeeType = ToCPtr->getPointeeType();
>> else if (const BlockPointerType *ToBlockPtr = ToType-
>> >getAsBlockPointerType())
>> ToPointeeType = ToBlockPtr->getPointeeType();
>> else
>> return false;
>
> These can use Type::getPointeeType(). Incidentally, please update
> the comment on Type::getPointeeType() to fix it to say that it works
> on block pointers. It does, but the comment says it doesn't.
>
Will do.
>>
>> QualType FromPointeeType;
>> - const PointerType *FromTypePtr = FromType->getAsPointerType();
>> - if (FromTypePtr)
>> - FromPointeeType = FromTypePtr->getPointeeType();
>> - else if (const BlockPointerType *FromBlockPtr
>> - = FromType->getAsBlockPointerType())
>> + if (const PointerType *FromCPtr = FromType->getAsPointerType())
>> + FromPointeeType = FromCPtr->getPointeeType();
>> + else if (const BlockPointerType *FromBlockPtr = FromType-
>> >getAsBlockPointerType())
>> FromPointeeType = FromBlockPtr->getPointeeType();
>> else
>> return false;
>
> These can use Type::getPointeeType().
>
>
>> + if (const ObjCObjectPointerType *FromPtrType =
>> + FromType->getAsObjCObjectPointerType())
>> + if (const ObjCObjectPointerType *ToPtrType =
>> + ToType->getAsObjCObjectPointerType()) {
>> + // Objective-C++ conversions are always okay.
>> + // FIXME: We should have a different class of conversions
>> for the
>> + // Objective-C++ implicit conversions.
>> + if (FromPtrType->isObjCIdType() || ToPtrType->isObjCIdType()
>> ||
>> + FromPtrType->isObjCClassType() || ToPtrType-
>> >isObjCClassType())
>
> Can use some sort of merged predicate for isObjCIdType ||
> isObjCClassType.
>
>>
>> +++ cfe/trunk/test/CodeGenObjC/encode-test.m Fri Jul 10 18:34:53 2009
>> @@ -1,7 +1,7 @@
>> // RUN: clang-cc -triple=i686-apple-darwin9 -fnext-runtime -emit-
>> llvm -o %t %s &&
>> // RUN: grep -e "\^{Innermost=CC}" %t | count 1 &&
>> -// RUN: grep -e "{Derived=#ib32b8b3b8sb16b8b8b2b8ccb6}" %t | count
>> 1 &&
>> -// RUN: grep -e "{B1=#@c}" %t | count 1 &&
>> +// RUN: grep -e
>> "{Derived=^{objc_class}ib32b8b3b8sb16b8b8b2b8ccb6}" %t | count 1 &&
>> +// RUN: grep -e "{B1=^{objc_class}@c}" %t | count 1 &&
>> // RUN: grep -e "v12 at 0:4\[3\[4@]]8" %t | count 1 &&
>> // RUN: grep -e "r\^{S=i}" %t | count 1 &&
>> // RUN: grep -e "\^{Object=#}" %t | count 1
>
> The output of @encode changed? Isn't this an ABI bug??
This following two issues are a result of moving away from the C-
structure dependency (which is a hack). More info...
We no longer treat "struct objc_class *" as synonymous with "Class".
This is a side-effect of removing
"ASTContext::isObjCClassStructType(T)".
@interface B1
{
struct objc_class *isa;
Int1 *sBase;
char c;
}
@end
Since it does effect GCC binary compatibility, we could certainly add
back a hack to make them synonymous.
>
>
>> +++ cfe/trunk/test/CodeGenObjC/overloadable.m Fri Jul 10 18:34:53
>> 2009
>> @@ -3,8 +3,8 @@
>>
>> @class C;
>>
>> -// RUN: grep _Z1fP11objc_object %t | count 1 &&
>> -void __attribute__((overloadable)) f(C *c) { }
>> +// RUN: grep _Z1fP2id %t | count 1 &&
>> +void __attribute__((overloadable)) f(id c) { }
>>
>> // RUN: grep _Z1fP1C %t | count 1
>> -void __attribute__((overloadable)) f(id c) { }
>> +void __attribute__((overloadable)) f(C *c) { }
>
> Likewise, mangling changing sounds like a serious ABI bug.
Same issue as above (but with ASTContext::isObjCIdStructType()). We
now mangle "id" as "id" (not the underlying structure).
Since it does effect GCC binary compatibility, we could certainly add
back a hack to make them synonymous.
>
>
>> =
>> =
>> =
>> =
>> =
>> =
>> =
>> =
>> =
>> =====================================================================
>> --- cfe/trunk/test/PCH/objc_exprs.m (original)
>> +++ cfe/trunk/test/PCH/objc_exprs.m Fri Jul 10 18:34:53 2009
>> @@ -6,7 +6,7 @@
>> // RUN: clang-cc -fblocks -include-pch %t -fsyntax-only -verify %s
>>
>> // Expressions
>> -int *A1 = (objc_string)0; // expected-warning {{'struct
>> objc_object *'}}
>> +int *A1 = (objc_string)0; // expected-warning {{aka 'id'}}
>
> Nice.
>
>> +++ cfe/trunk/test/SemaObjC/comptypes-5.m Fri Jul 10 18:34:53 2009
>> @@ -26,8 +26,8 @@
>> MyOtherClass<MyProtocol> *obj_c_super_p_q = nil;
>> MyClass<MyProtocol> *obj_c_cat_p_q = nil;
>>
>> - obj_c_cat_p = obj_id_p; // expected-warning {{incompatible
>> type assigning 'id<MyProtocol>', expected 'MyClass *'}}
>> - obj_c_super_p = obj_id_p; // expected-warning {{incompatible
>> type assigning 'id<MyProtocol>', expected 'MyOtherClass *'}}
>> + obj_c_cat_p = obj_id_p;
>> + obj_c_super_p = obj_id_p;
>
> Is this supposed to be allowed?
GCC warns...
test/SemaObjC/comptypes-5.m:29: warning: assignment from distinct
Objective-C type
test/SemaObjC/comptypes-5.m:30: warning: assignment from distinct
Objective-C type
I decided to allow it. Rationale: Both MyClass and MyOtherClass
implement MyProtocol. Since the protocols "match", and you can assign
any 'id' to an interface type (without warning), I decided to allow
it. I'm happy to put back the warning if others feel strongly
(Fariborz?).
>
>
>> +++ cfe/trunk/test/SemaObjC/conditional-expr-3.m Fri Jul 10
>> 18:34:53 2009
>> @@ -59,7 +59,7 @@
>> }
>>
>> void f10(int cond, id<P0,P1> x0, id<P0,P2> x1) {
>> - barP2(cond ? x0 : x1);
>> + barP2(cond ? x0 : x1); // expected-warning {{incompatible type
>> passing 'id<P0,P1>', expected 'id<P2>'}}
>
> Shouldn't the merged type of ?: be id<P0> here, not id<P0,P1> ?
Need to research...
>
>
>> +++ cfe/trunk/test/SemaObjC/message.m Fri Jul 10 18:34:53 2009
>> @@ -95,6 +95,6 @@
>> void foo4() {
>> struct objc_object X[10];
>>
>> - [X rect];
>> + [(id)X rect];
>> }
>
> This is a bug, we should be performing unary "array -> pointer"
> decay here. This was in response to a bugzilla, we need to support
> this.
I don't see the issue here. Both GCC and clang warn if no cast is used.
[steve-naroffs-imac-3:~/llvm/tools/clang] snaroff% cc -c xx.m
xx.m: In function ‘foo4’:
xx.m:29: warning: invalid receiver type ‘objc_object [10]’
[steve-naroffs-imac-3:~/llvm/tools/clang] snaroff% ../../Debug/bin/
clang -c xx.m
xx.m:29:3: warning: receiver type 'struct objc_object *' is not 'id'
or interface pointer, consider casting it to 'id'
[X rect];
^~
xx.m:29:3: warning: method '-rect' not found (return type defaults to
'id')
[X rect];
^~~~~~~~
2 diagnostics generated.
>
>
>> +++ cfe/trunk/test/SemaObjCXX/overload.mm Fri Jul 10 18:34:53 2009
>> @@ -1,4 +1,5 @@
>> // RUN: clang-cc -fsyntax-only -verify %s
>> +// XFAIL
>> @interface Foo
>> @end
>
> What is the problem with this test?
There were several problems (mostly related in incomplete handling of
ObjC types in the C++ infrastructure). I discussed this with Doug and
we decided to add the XFAIL.
>
>
> Thanks again for working on this Steve! I know it's largely
> thankless, but it's a huge cleanup.
I appreciate the encouragement. I think this kind of hygiene is quite
important. Kind of like getting my teeth cleaned:-)
snaroff
>
>
> -Chris
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20090713/86bf36cc/attachment.html>
More information about the cfe-commits
mailing list