[cfe-commits] r113397 - in /cfe/trunk: include/clang/AST/Type.h lib/AST/Type.cpp lib/Sema/SemaType.cpp test/SemaObjC/legacy-objc-types.m test/SemaObjCXX/legacy-objc-types.mm

[Fariborz Jahanian]

On Sep 8, 2010, at 3:35 PM, John McCall wrote:

>
> On Sep 8, 2010, at 3:10 PM, Fariborz Jahanian wrote:
>
>>
>> On Sep 8, 2010, at 2:59 PM, John McCall wrote:
>>
>>> On Sep 8, 2010, at 2:30 PM, Fariborz Jahanian wrote:
>>>> On Sep 8, 2010, at 2:04 PM, John McCall wrote:
>>>>> If I understand this patch correctly, there's a crash arising  
>>>>> from invalid redeclarations of functions.  If that's true, this  
>>>>> is almost certainly a workaround rather than a fix, because it's  
>>>>> just making the redeclaration valid.  Please fix the underlying  
>>>>> crash rather than working around it like this.
>>>>
>>>> Underlying crash is in IRgen because we do not catch such  
>>>> duplicate function definitions. This patch makes the re- 
>>>> declaration  identical to previous one and forces error in Sema.
>>>
>>> Oh, I see, because they mangle the same way.
>>>
>>> I think the appropriate place to make this change is in  
>>> Sema::FunctionArgTypesAreEqual, in SemaOverload.cpp.  We already  
>>> have similar logic for redeclaring functions with different  
>>> protocol-qualified types.
>>
>> This was my first try :). Then I discovered that it prevents  
>> declaration followed by definition, as in:
>> id f(id ptr);
>> objc_object* f(id ptr) { }
>>
>> So, the patch.
>
> By 'prevents' do you mean that it produces an error or that it  
> doesn't produce an error and IR gen crashes on it?  Because this  
> *should* be an error unless we start accepting objc_object as  
> builtin-id.

Oops. Correcting test I ran into:

void f(id ptr);
void f(objc_object* ptr) {}

After the change, clang issued error in Sema (about duplicate  
declaration). This should not be an error.
>
>
>>>> 1. Do nothing and let the test case crash ( I think it was a  
>>>> manufactured test ).
>>>> 2. Accept the alternative 'struct ...' as builtin type everywhere.
>>>> 3. Issue error if it is used anywhere.
>>>> 4.???
>>>
>>> Personally I like #3 because ObjC code using 'struct objc_object'  
>>> is very likely to either not compile or have really unfortunate  
>>> behavioral changes — I think Argyrios recently hunted down  
>>> something in the latter category — but that's the sort of problem  
>>> we can only suss out with SWBs.  For the immediate problem,  
>>> changing FunctionArgTypesAreEqual should be enough.
>> This is a policy decision. I am open to it.
>
> Okay.  Can you file a radar to track this discussion?
Radar 8400356 is it.

- Fariborz

>
>
> John.