steakhal added a comment. T
In D152269#4403491 <https://reviews.llvm.org/D152269#4403491>, @tripleCC wrote: > In D152269#4403404 <https://reviews.llvm.org/D152269#4403404>, @steakhal > wrote: > >> In D152269#4403282 <https://reviews.llvm.org/D152269#4403282>, @tripleCC >> wrote: >> >>> Yes, your changes are aligned with my intent. It seems like you have made >>> excellent optimizations to this patch. >>> To eliminate the following two warnings, I add the `-fobjc-arc`(enable >>> Automatic Reference Counting) compilation option for NSContainers test . >>> >>> + MyView *view = b ? [[MyView alloc] init] : 0; // expected-warning >>> {{Potential leak of an object stored into 'view'}} >>> + NSMutableArray *subviews = [[view subviews] mutableCopy]; // >>> expected-warning {{Potential leak of an object stored into 'subviews'}} >> >> I think we shouldn't add the `-fobj-arc` as these two new issues are >> considered TPs, and meaningful for the test file they are part of. It's just >> that they are not that meaningful in the scope of this patch, but that >> shouldn't hold us back from improving what the test covers and demonstrates, >> so I'm fine if those two appear as part of this patch. >> >>> I will continue to contribute more ObjC-related fixes in the future, and >>> currently, my work is also related to this. >> >> Sounds good. Thanks for clarifying. >> If that's the case, after a few more quality patches I think it would make >> sense to request you commit access. Let's keep this in mind now. >> >>> Thank you very much for your review. Would you mind if I merge your >>> recommendations? >> >> I don't mind. You don't need to give attribution. Keep parts of the whole as >> you wish. > > Thanks. > > By the way , there are already other unit tests focus on leak scenarios, such > as the retain-release*.m test cases. Should we focus on testing container > issues here? Currently, Objective-C code almost always uses ARC (Automatic > Reference Counting). Both makes sense. You can probably better judge this. I'm totally fine with any of the two options. One note, clang-format the affected lines. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D152269/new/ https://reviews.llvm.org/D152269 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
