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. >> Also, there are several problems with this pattern of recognizing 'struct >> objc_object' and 'struct objc_class'. >> 1) This is a significant change in behavior; previously we intentionally >> never recognized these types as being their respective builtin types. I'm >> actually ambivalent about that — we should probably recognize this, at the >> very least to give an error about it — but we shouldn't change the design >> lightly just to fix a crash. > > I am open to issuing error on use of such types. But there could be many > situations which previous gcc compiled code will break. Yeah, that's certainly possible. >> 2) If we do want to recognize these structs as the builtin types, we should >> do consistently, not just for parameters, and not just for the exact case of >> pointers to the type. The cleanest way to do that is in >> ConvertDeclSpecToType. > > I thought about this at first. But we don't want to encourage use of these > types and make them work silently. I rather let compiler issues error to > discourage their use. Right, but since this logic isn't constrained to just redeclaration checks, it's already going to do that. >> 3) The logic to recognize these structs should respect context; only >> structs in global scope are the builtins, i.e. if >> getDeclContext()->getRedeclContext()->isTranslationUnit() (there might be a >> convenience function for that somewhere). > > I agree on this point. I can tighten up the type checking. > In any case, point of this patch was to fix the crash in a limited case and > not treat 'struct objc_object' as a replacement for 'id' everywhere. > So, these are alternatives. > > 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. John. _______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
