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

[Fariborz Jahanian]

Addition of a category with protocol qualifier MyProtocol should not  
imply that type of obj_c_cat_p
is now MyClass<MyProtocol>* IMO. Having said that, this is a corner  
case. So please file a radar
and run it by the objc runtime group. We will deal with it as issue is  
resolved.

- Fariborz

On Jul 23, 2009, at 9:00 AM, steve naroff wrote:

> Fariborz,
>
> I want to follow-up on an issue we discussed a week ago. See below...
>
> On Jul 13, 2009, at 5:14 PM, Fariborz Jahanian wrote:
>>>
>>>>>>
>>>>>> 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.
>>>>
>>>> We should probably put the warning back.  Lack of a warning looks  
>>>> like a clang bug.  For example, clang (and gcc)
>>>> both issue warning here:
>>>>
>>>> @protocol MyProtocol
>>>> @end
>>>>
>>>> @interface MyClass
>>>> @end
>>>>
>>>> int main()
>>>> {
>>>> id <MyProtocol> obj_id_p;
>>>> MyClass *obj_c_cat_p;
>>>>
>>>> obj_c_cat_p = obj_id_p;
>>>> return 0;
>>>> }
>>>>
>>>> % $CLANGCC good.m
>>>> good.m:12:15: warning: incompatible type assigning  
>>>> 'id<MyProtocol>', expected 'MyClass *'
>>>> obj_c_cat_p = obj_id_p;
>>>>
>>>> But if you add a category conforming to the protocol (as in the  
>>>> test), then warning disappears in clang. Categories do not
>>>> enhance or change the type so behavior should not change.
>>>>
>>>
>>> Formally, I see your point (i.e. categories don't actually modify  
>>> the type). Nevertheless, categories allow methods to be added to a  
>>> class (a semantic that the compiler is very aware of). Since both  
>>> categories and protocols are nothing more than "method  
>>> containers", it seems like the compiler should be silent it  
>>> determines the message can be sent. With dynamic typing and  
>>> forwarding, all of this is speculative anyway:-) Just to be  
>>> clear...I don't feel very strongly about this. I just think we  
>>> should do what makes sense (and is in the spirit of the language).  
>>> In this instance, I don't think GCC compatibility is a big issue...
>>
>> I agree your rational makes sense. But, let's not change this  
>> behavior. Our users are unforgiving when we remove their favorite  
>> warnings :).
>>
>
> Yesterday, I cleaned up code related to this issue. The cleanup  
> ended up fixing a few issues that were clearly wrong (and in some  
> cases were already noted in the tests).
>
> Now that the code has been "tightened up", I believe it's even more  
> difficult to "bring back" this diagnostic above (related to SemaObjC/ 
> comptypes-5.m). ASTContext::ClassImplementsProtocol() clearly  
> considers protocols on categories implemented by the class.
>
> Since I don't think there is a compelling case for a warning, I'm  
> going to leave it as is (without a warning). From my perspective,  
> the code is valid (and I believe GCC is incorrectly producing a  
> warning).
>
> If you can come up with a clean way to bring back the warning, feel  
> free. I've been fiddling with this and can't come up with an  
> acceptable solution (without breaking valid code, which is bad:-).
>
> For reference, I've included the diff of the test case below...
>
> Thanks,
>
> snaroff
>
> [steve-naroffs-imac-3:tools/clang/test] snaroff% svn diff
> Index: SemaObjC/comptypes-5.m
> ===================================================================
> --- SemaObjC/comptypes-5.m	(revision 76877)
> +++ SemaObjC/comptypes-5.m	(working copy)
> @@ -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;
> -  obj_c_super_p = obj_id_p;
> +  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_id_p = obj_c_cat_p;  /* Ok */
>   obj_id_p = obj_c_super_p; /* Ok */
>
>
>>>
>>>
>>> My 2$,
>>
>> My 2cents :).
>>
>> - fariborz
>>
>>
>