> Why not get the IdentifierInfo pointer for the class name from the NSAPI > object and compare that ? There are more than one way to do things, it’s just lack of knowledge about the code base.
> Also there is code duplication, since the same code pattern is used in 3 > places, could you refactor into a function ? Fixed this as well. Also, I got rid of `NSCountedSet` since it’s a subclass of `NSMutableSet` and will be caught by `isSubclassOfNSClass`. The new version of the patch attached. -- AlexDenisov Software Engineer, http://lowlevelbits.org
proper_fix_for_circular_containers_v2.patch
Description: Binary data
> On 05 Aug 2015, at 19:22, Argyrios Kyrtzidis <kyrtzi...@apple.com> wrote:
>
>>
>> - if (S.NSMutableArrayPointer != Message->getReceiverType()) {
>> + ObjCInterfaceDecl *Receiver = Message->getReceiverInterface();
>> + if (!Receiver) {
>> + return None;
>> + }
>> +
>> + bool IsMutableArray = false;
>> + do {
>> + QualType QT = S.Context.getObjCInterfaceType(Receiver);
>> + QualType ReceiverType = S.Context.getObjCObjectPointerType(QT);
>> +
>> + IsMutableArray = !S.NSMutableArrayPointer.isNull() &&
>> + ReceiverType == S.NSMutableArrayPointer;
>> +
>> + if (IsMutableArray) {
>> + break;
>> + }
>> + } while ((Receiver = Receiver->getSuperClass()));
>> +
>> + if (!IsMutableArray) {
>> return None;
>> }
>>
>
> Why not get the IdentifierInfo pointer for the class name from the NSAPI
> object and compare that ? It seems unnecessary to be doing the type lookups
> for this.
>
> Also there is code duplication, since the same code pattern is used in 3
> places, could you refactor into a function ?
>
>
>
>> On Jul 31, 2015, at 1:55 PM, AlexDenisov <1101.deb...@gmail.com> wrote:
>>
>>> To clarify, are you saying that the warning may lead to false positives
>>> when used in subclasses ?
>>
>> Seems I was wrong.
>> Just checked the behaviour with backing storage - it also leads to a
>> circular container problem.
>>
>> Also, you can find attachment with a ‘proper’ implementation, which also
>> covers subclassing.
>>
>> P.S. I didn’t measure performance, but I think this implementation might
>> have negative impact on the speed.
>> --
>> AlexDenisov
>> Software Engineer, http://lowlevelbits.org
>>
>> <proper_fix_for_circular_containers.patch>
>>> On 30 Jul 2015, at 18:18, Argyrios Kyrtzidis <kyrtzi...@apple.com> wrote:
>>>
>>>
>>>> On Jul 30, 2015, at 1:05 AM, AlexDenisov <1101.deb...@gmail.com> wrote:
>>>>
>>>> The patch is a simplest fix for crash when CheckObjCCircularContainer
>>>> applies to a message to a ’super’, e.g.:
>>>>
>>>> @implementation Foo : NSMutableArray
>>>> - foo {
>>>> [super addObject:nil];
>>>> }
>>>> @end
>>>>
>>>>
>>>> This is, probably, not a proper fix for the problem,
>>>> but initial patch wasn’t intended to apply checks to any kind
>>>> of subclassing, because it, imho, over-complicates implementation:
>>>
>>> To clarify, are you saying that the warning may lead to false positives
>>> when used in subclasses ?
>>> If that’s the case could we just disable it inside collection subclasses,
>>> at least until the false positives can be addressed ?
>>>
>>>>
>>>> This particular problem touches subclassing from a class-cluster,
>>>> which means that the concrete subclass will have some backing storage,
>>>> e.g.:
>>>>
>>>> @implementation FootableArray : NSMutableArray
>>>> {
>>>> NSMutableArray *_backingStorage;
>>>> }
>>>>
>>>> - addObject:(id)object {
>>>> [_backingStorage addObject:object];
>>>> }
>>>>
>>>> @end
>>>>
>>>> In this case even adding `self` to `self` would not lead to a circular
>>>> container:
>>>>
>>>> - foo {
>>>> [self addObject:self]; // puts `self` into the `_backingStorage`
>>>> }
>>>>
>>>> I would apply this patch as is and postpone a ‘proper and bullet-proof
>>>> implementation’
>>>> when I, or somebody who is also interested, will have more time.
>>>>
>>>> If there are any questions/suggestions/objections - let’s discuss them.
>>>> --
>>>> AlexDenisov
>>>> Software Engineer, http://lowlevelbits.org
>>>>
>>>>
>>>> <fix_circular_container_crash.patch>
>>>
>>
>
signature.asc
Description: Message signed with OpenPGP using GPGMail
_______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
