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

On Jul 13, 2009, at 4:18 PM, Chris Lattner wrote:
>
>>>> 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.

I'm sure there are places where block pointers should be included.

I agree that a predicate would be helpful (if we can come up with a  
clear name/semantic).

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

I see...I guess I read your response too quickly (sorry for the  
confusion).

clang allows this (for GCC compatibility). I'd have no problem  
rejecting code like this (since it is silly):

@interface XX
-self;
@end

static void func() {

   XX *x;

   [x+4 self];
}

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

Agreed...will consider.

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

I agree. Will do.

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

O.K...will reintroduce a hack to treat "struct objc_object" and  
"struct objc_class" specially. Oh well:-)

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

Will do.

>>
>>>
>>>
>>>> +++ 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/59688f71/attachment.html>