> + IdentifierInfo *II = &S.Context.Idents.get(InterfaceDecl->getName());
You can just do “InterfaceDecl->getIdentifier()”, no need to lookup by string. Also this will eliminate the need to pass Sema as parameter. And ‘isSubclassOfNSClass()’ seems generally useful, how about you make it a function of NSAPI ? Otherwise LGTM. > On Aug 5, 2015, at 1:11 PM, AlexDenisov <1101.deb...@gmail.com> wrote: > >> 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> >> 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> >>>> >>> >> > _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
