On Aug 20, 2012, at 6:47 PM, Jordan Rose wrote: > Yeah, I don't like this approach as much as I thought I would. My original > reason for making a new summary out of an old one was to re-use the existing > summary evaluation code. This eliminates that, and processSummaryOfInlined is > crippled since it only handles StopTrackingHard and NoRetHard. And there's a > lot of duplication elsewhere. > > I'd rather see this: > > RetainSummaryManager &Summaries = getSummaryManager(C); > const RetainSummary *Summ = Summaries.getSummary(Call, C.getState()); > > if (C.wasInlined && !Summ->appliesToInlined()) > return; > > checkSummary(*Summ, Call, C); > > The only difference between the "hard" and "soft" versions is that NoRetHard > means /remove/ RefBindings, while NoRet means don't touch them. This could be > renamed to "Untracked" or "UntrackedSymbol". > > > - void updateSummaryForCall(const RetainSummary *&Summ, > - const CallEvent &Call); > + void updateForCallbackArgument(const RetainSummary *&Summ, > + const CallEvent &Call); > > This is not a core point about the functionality of this patch, but I think > this is an inferior interface. The idea behind 'updateSummaryForCall' is that > it takes a context-less summary derived from an ObjCMethodDecl, or even just > a selector, and adds the context of the arguments and possibly additional > information about the receiver besides its type. The summary generation > before this point can be cached and re-used if we call the same method later, > but updateSummaryForCall is the time to make changes specific to this > CallEvent. > > Currently, the only such change is the presence or absence of a callback > parameter, but there's no reason that can't change in the future. Keeping > this as 'updateSummaryForCall' gives a proper opaque effect: given a summary > for a given decl, update it for this particular call. getSummary shouldn't be > thinking about the various ways in which a CallEvent could affect a base > summary. >
OK. Thanks for the background. I'll change the name back to what it was. (The name was so generic and what the function is doing is very specific, but I see your argument.) > I have some spot comments, but I'll save those for the next iteration of the > patch, or perhaps tomorrow morning. > > Jordan > > > On Aug 20, 2012, at 16:59 , Anna Zaks <[email protected]> wrote: > >> This resolves retain count checker false positives that are caused by >> inlining ObjC and other methods. Essentially, if we are passing an object to >> a method with "delegate" in the selector or a function pointer as another >> argument, we should stop tracking the other parameters/return value as far >> as the retain count checker is concerned. >> >> Submitting as a patch per Jordan's request. >> >> Anna. >> <Retain_Count_Stop_Tracking_Even_If_Inlined.diff> > _______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
