On Apr 2, 2013, at 6:42 PM, Jordan Rose <[email protected]> wrote:
> > On Apr 2, 2013, at 10:11 , Anna Zaks <[email protected]> wrote: > >> >> On Apr 1, 2013, at 6:44 PM, Jordan Rose <[email protected]> wrote: >> >>> >>> On Apr 1, 2013, at 18:28 , Anna Zaks <[email protected]> wrote: >>> >>>> + // Note: the last argumet is false here because these are >>>> + // non-top-level regions. >>> >>> Typo: "agument". >>> >>> >>> This is fine for now, but I hope we can kill the three sets of >>> regions...they're already a bit hard to follow. >>> >>> - top-level non-const regions (passed to checkRegionChanges and >>> checkPointerEscape) >>> - all non-const regions (passed to checkRegionChanges and >>> checkPointerEscape) >>> - top-level const regions (passed to checkPointerEscape's const variant) >>> >>> - all const regions (we currently don't model this but we might some day) >>> >>> I guess we save a little bit of effort by caching the top-level regions in >>> a list instead of extracting them from the CallEvent again, >> >> I did implement the other approach first - where we construct the top level >> sets in users when they need them instead of passing them around. The code >> looked much more complex and less maintainable. So I've reimplemented this >> to let RegionStore populate those regions. I like it much better. Here are >> the additional benefits to the one mentioned above: >> - This way the abstraction is right - the users rely on the info from >> RegionStore invalidation authority, instead of recalculating the top-level >> sets themselves. >> - Top level regions are not only call arguments, but should also include >> "extra invalidated values" (self and this), so their computation is complex. >> - If we ever add a new top level region, we don't need to change the logic >> in multiple places. >> >>> but I'm not sure it's worth the additional complexity in either >>> notifyCheckersOfPointerEscape or checkRegionChanges. >> >> ? With the current approach we added complexity only to RegionStore, instead >> of adding it to the users. notifyCheckersOfPointerEscape has to >> differentiate between the regions. Hopefully, checkRegionChanges can be >> completely removed after we remove the calls to it from RetainCountChecker; >> and migrate the checker to use pointer escape callback. After that is done, >> none of the checkers will have to reason about the regions. > > "Top-level regions" doesn't have any actual meaning—it's only used for > RetainCountChecker, and when I invented it for that it meant "arguments, plus > the receiver I guess". But I see your point that that "plus the receiver" is > more work than callers should have to do. Thanks for the explanation of the > thought process. > > (Maybe I'd be happier if the top-level and non-top-level sets were tied > together in a struct, but that can wait until we support non-top-level const > regions.) > Yes, wrapping these in a struct would be better. However, RetainCountChecker aside, there is only notifyCheckersOfPointerEscape that consumes the extra region (and it does need to know the difference between top-level and not). > Jordan
_______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
