[cfe-commits] r117271 - in /cfe/trunk: lib/Sema/SemaDeclObjC.cpp test/SemaObjC/class-conforming-protocol-2.m test/SemaObjC/method-conflict-1.m test/SemaObjC/method-typecheck-3.m

[John McCall]

On Oct 25, 2010, at 2:28 PM, David Chisnall wrote:
> On 25 Oct 2010, at 18:54, John McCall wrote:
>> On Oct 25, 2010, at 10:23 AM, David Chisnall wrote:
>>> Author: theraven
>>> Date: Mon Oct 25 12:23:52 2010
>>> New Revision: 117271
>>> 
>>> URL: http://llvm.org/viewvc/llvm-project?rev=117271&view=rev
>>> Log:
>>> Only warn for mismatched types in Objective-C methods when they are incompatible, not when they are simply different.  Now we test whether the difference in types breaks the principle of substitutability, rather than whether they are different.
>> 
>> I know there isn't a spec for Objective-C or anything, but I think changes which change basic language behaviors should probably be discussed before committing them.  
> 
> I agree.  This doesn't change the language semantics at all, however.  It merely makes an overly pedantic warning into a less-pedantic one.  I came across this because we compile a lot of code with -Werror, and gcc accepts things that clang is rejecting.  I think this is because gcc, by default, does not check that types match at all (which is much worse behaviour because it can miss things that can cause stack corruption with the Apple or GCC runtimes).  

Right.

> My aim was to make the warning do what warnings should do - tell you when you are doing something that is valid, but probably not what you want.  It now catches some cases that gcc was ignoring in our code where we were doing the wrong thing (i.e. overriding a method with distinct types), but 

Yeah, I agree that this is the right direction;  in fact, I'd love it if this change were sufficient to turn the associated diagnostic into a hard error.  We'd need to extensively qualify that, though.

>> That said, I really like this change, and I don't see any serious problems as long as the idea is only applied to Objective-C pointers and Objective-C methods.  
> 
> Yes, it wouldn't make sense with C++ objects or with C types.  C functions are not overridden in this manner, and have well-defined requirements, as do C++ methods.

Okay.

>> My only worry is how it affects (1) sends to id and (2) possible future directions.
> 
> I had some concerns about id as well, but I think it's approximately sane now.  I'm not completely sure that the current behaviour is ideal.

Right; we clearly want to move towards a world where the language does not force programmers to use 'id' so often.

The issue with sends to 'id' that I was thinking about is that there's an extremely loose sort of type-checking applied to them:  if the given selector has only one known signature, the call follows that.  My worry is that this can introduce multiple signatures and thus foil type-checking, whereas clearly the right behavior is to use the most general known signature.

> I'm not sure that there are any future directions - I'm just making the warning a bit tighter in its checking.

There are some internal discussions in flight about ways to avoid 'id' in certain extremely common cases.

>> Anyway, we'll talk it over with the Apple runtime people;  I think we can make a pretty compelling case.
> 
> It shouldn't matter to them - it doesn't affect code generation, or even which programs are accepted, it just makes some programs compile without warnings that had spurious warnings before.

I should clarify that the runtime team here also guides the design of the language.

We have an internal deadline coming up, and it seems that we'd prefer not to make this change for it, so I'm going to introduce a specialized warning for covariant/contravariant overrides which will need to be enabled on Darwin.  Whether it's on by default and you have to specifically disable it or it's off by default and Darwin triples specifically enable it is something I'm still ambivalent about.

John.