On Apr 2, 2013, at 19:48 , Anna Zaks <[email protected]> wrote: > > 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).
Well, in theory...in practice, no one's taking advantage of it!
_______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
