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

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