[PATCH] [ObjC] New warning: circular containers

[AlexDenisov]

My point was that this check needs to be applied for both ARC and non-ARC code.
Non-ARC code: weird behaviour, ARC code: weird behaviour + retain cycle.
Anyway, going to commit, if it’s ok.
-- 
AlexDenisov
Software Engineer, http://alexdenisov.github.io

On 4 Mar 2015 at 18:13:10, jahanian (fjahanian at apple.com) wrote:


On Mar 4, 2015, at 6:50 AM, AlexDenisov <1101.debian at gmail.com> wrote:

> No. I meant add expected-note which will point to the declaration as part of the diagnostic.
Got it. New patch attached.

> You mean you cannot add it after checkRetainCycles(Result); as in:
I can, but then it would be skipped for non-arc code, which still has this problem (not retain cycle, but ‘corruption’ at runtime).
— 

IIRC, you are saying that this intervening code:
if (!isImplicit && Method) {
      if (const ObjCPropertyDecl *Prop = Method->findPropertyDecl()) {
        bool IsWeak =
          Prop->getPropertyAttributes() & ObjCPropertyDecl::OBJC_PR_weak;
        if (!IsWeak && Sel.isUnarySelector())
          IsWeak = ReturnType.getObjCLifetime() & Qualifiers::OCL_Weak;
        if (IsWeak &&
            !Diags.isIgnored(diag::warn_arc_repeated_use_of_weak, LBracLoc))
          getCurFunction()->recordUseOfWeak(Result, Prop);
      }
    }
  }

is necessary for non-arc code. I don’t see it but it is not a major point.
Ok to check in.
- Thanks, Fariborz

AlexDenisov
Software Engineer, http://alexdenisov.github.io

On 3 Mar 2015 at 22:27:09, jahanian (fjahanian at apple.com) wrote:


On Mar 3, 2015, at 12:33 PM, AlexDenisov <1101.debian at gmail.com> wrote:

> Provide a note where receiver/argument has been declared.
Do you mean ‘add comments’?

No. I meant add expected-note which will point to the declaration as part of the diagnostic.


> Place CheckObjCCircularContainer(Result) right after checkRetainCycles(Result).
It still causes weird behaviour even without ARC. Of course there is no retain cycle anymore, but app still hangs with recursion/crash.


You mean you cannot add it after checkRetainCycles(Result); 
as in:

checkRetainCycles(Result);
CheckObjCCircularContainer(Result) ;

if (!isImplicit && Method) {
      if (const ObjCPropertyDecl *Prop = Method->findPropertyDecl()) {
        bool IsWeak =
          Prop->getPropertyAttributes() & ObjCPropertyDecl::OBJC_PR_weak;
        if (!IsWeak && Sel.isUnarySelector())
          IsWeak = ReturnType.getObjCLifetime() & Qualifiers::OCL_Weak;
        if (IsWeak &&
            !Diags.isIgnored(diag::warn_arc_repeated_use_of_weak, LBracLoc))
          getCurFunction()->recordUseOfWeak(Result, Prop);
      }
    }
  }

 return MaybeBindToTemporary(Result);
}
 


> This patch does not address the general case of same expression used as receiver and addObject argument.
> Is this something that you care enough to address?
Do you mean something like ’[self.array addObject:self.array]’?
If so, then it doesn’t really makes sense, because we can’t ensure that returned objects are the same, there’ll be false positives.

Ok,
- Fariborz

-- 
AlexDenisov
Software Engineer, http://alexdenisov.github.io

On 3 Mar 2015 at 20:46:15, jahanian (fjahanian at apple.com) wrote:


On Mar 3, 2015, at 11:02 AM, jahanian <fjahanian at apple.com> wrote:

Patch looks good with couple of minors.
Provide a note where receiver/argument has been declared.
Place CheckObjCCircularContainer(Result) right after checkRetainCycles(Result).

This patch does not address the general case of same expression used as receiver and addObject argument.
Is this something that you care enough to address? Need not be in this patch though.


- Fariborz



_______________________________________________ 
cfe-commits mailing list 
cfe-commits at cs.uiuc.edu 
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits

<objc_circular_container.patch>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20150304/ac2e7777/attachment.html>