[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

[John McCall]

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.

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

John.