[cfe-commits] r169571 - /cfe/trunk/lib/StaticAnalyzer/Core/RegionStore.cpp

[Ted Kremenek]

On Dec 7, 2012, at 9:36 AM, Jordan Rose <jordan_rose at apple.com> wrote:

>> 
>>  // BindDefault is only used to initialize a region with a default value.
>>  StoreRef BindDefault(Store store, const MemRegion *R, SVal V) {
>> -    RegionBindings B = GetRegionBindings(store);
>> -    assert(!lookup(B, R, BindingKey::Default));
>> -    assert(!lookup(B, R, BindingKey::Direct));
>> -    return StoreRef(addBinding(B, R, BindingKey::Default, V)
>> -                      .getRootWithoutRetain(), *this);
>> +    RegionBindingsRef B = getRegionBindings(store);
>> +    assert(!B.lookup(R, BindingKey::Default));
>> +    assert(!B.lookup(R, BindingKey::Direct));
>> +    return StoreRef(B.addBinding(R, BindingKey::Default, V)
>> +                     .asImmutableMap()
>> +                     .getRootWithoutRetain(), *this);
> 
> I was thinking about this; I think we should have a makeStoreRef(RegionBindingsRef) helper, since we see this pattern so much.
> 

I agree.

> 
>>        // invalidate that region.  This is because a block may capture
>>        // a pointer value, but the thing pointed by that pointer may
>>        // get invalidated.
>> -        Store store = B.getRootWithoutRetain();
>> +        Store store = B.asImmutableMap().getRootWithoutRetain();
>>        SVal V = RM.getBinding(store, loc::MemRegionVal(VR));
> 
> This is an unfortunate impedance mismatch.

Impedance mismatch?  Please elucidate.

> 
> 
>> StoreRef RegionStoreManager::BindArray(Store store, const TypedValueRegion* R,
>> @@ -1724,9 +1813,9 @@
>> 
>>  // There may be fewer values in the initialize list than the fields of struct.
>>  if (FI != FE) {
>> -    RegionBindings B = GetRegionBindings(newStore.getStore());
>> -    B = addBinding(B, R, BindingKey::Default, svalBuilder.makeIntVal(0, false));
>> -    newStore = StoreRef(B.getRootWithoutRetain(), *this);
>> +    RegionBindingsRef B = getRegionBindings(newStore.getStore());
>> +    B = B.addBinding(R, BindingKey::Default, svalBuilder.makeIntVal(0, false));
>> +    newStore = StoreRef(B.asImmutableMap().getRootWithoutRetain(), *this);
>>  } 
> 
> Same impedance mismatch in BindStruct, BindVector, and BindArray. For that matter, it's worse: we know that all the bindings will happen in a single cluster.

Not sure what you mean by impedance mismatch.  I do agree that asImmutableMap().getRootWithoutRetain() chain is gross, and can be sugared up.  Is that what you mean?
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20121207/7e1e2bc9/attachment.html>