r178518 - [analyzer] Teach invalidateRegions that regions within LazyCompoundVal need to be invalidated

[Anna Zaks]

On Apr 2, 2013, at 6:42 PM, Jordan Rose <jordan_rose at apple.com> wrote:

> 
> On Apr 2, 2013, at 10:11 , Anna Zaks <ganna at apple.com> wrote:
> 
>> 
>> On Apr 1, 2013, at 6:44 PM, Jordan Rose <jordan_rose at apple.com> wrote:
>> 
>>> 
>>> On Apr 1, 2013, at 18:28 , Anna Zaks <ganna at apple.com> 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

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