[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/

Chris Lattner clattner at apple.com
Mon Jul 13 13:18:24 PDT 2009


On Jul 13, 2009, at 9:14 AM, steve naroff wrote:
>> 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.

Ok.

>> 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.

Ok, going with something simple makes sense to me.

>>> 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()?

Works for me.

>>> 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.

What about block pointers?  getAsPointeeType works with blocks, are  
there places that really want to be checking for "c pointer, objc  
pointer, or block pointer"?  If so, it would make sense to introduce a  
predicate for it.  To address Sebastian's concern, we would just need  
to name the predicate right.

>>>
>>> +++ 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.

I mean something like:

NSString *X;
   X+4;

>> 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:-)

Ok.

>> 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).

Of course, no harm delaying the cleanups.

>> 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).

Right, none of my comments really were meant to imply that you should  
have included them in the original patch, they were meant for follow  
up changes.

>>> @@ -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.

The point of using the predicate is that it allows merging of these  
two if blocks.

>>> @@ -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:-)

Sure, a better name than "isAnyPointerType" is definitely appreciated,  
but I think it is useful to consider it.  There is a bunch of code  
that uses this predicate now.

>>>    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).

Ok, I completely missed that.  This is really subtle, why not set  
"PointeeTy" with a call to the getPointeeType() method, and then  
change the places that look at PTy/OPT to query the original type  
directly?  I think the code would be more clear that way.

>>> +++ 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.

I think that we should do this for @encode and C++ mangling.   
Compatibility is very important at this level because it isn't a  
matter of the compiler rejecting or producing a warning, it is a  
silent miscompilation.

>>
>>> +++ 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.

Ok, please do.

>>> +++ 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?).

I'll let Fariborz make this call.

>>>
>>> +++ 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.

Ok, then please fix to test by adding an 'expected-warning' instead of  
inserting the explicit cast.  The intent of the test is to verify that  
passing an array as the receiver works.

>
>>
>>
>>> +++ 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.

Ok, works for me.

>> 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:-)

:)

-Chris
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20090713/43e64841/attachment.html>


More information about the cfe-commits mailing list