On Apr 2, 2013, at 8:46 PM, Jordan Rose <[email protected]> wrote:
> > 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! > I see. So ideally, we would replace the code in RetainCountChecker to use escape callback (assuming it can deal with regions escaping into structs). Than, we would write custom code there to differentiate between arguments + self, and not. Finally, we would remove the top level from everywhere else. Correct?
_______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
