A lot touched, but seems reasonable. A few comments inline, though not many of them are actionable.
On Dec 6, 2012, at 17:55 , Ted Kremenek <[email protected]> wrote: > Author: kremenek > Date: Thu Dec 6 19:55:21 2012 > New Revision: 169571 > > URL: http://llvm.org/viewvc/llvm-project?rev=169571&view=rev > Log: > Change RegionStore to always use ImmutableMapRef for processing cluster > bindings. > > This reduces analysis time by 1.2% on one test case (Objective-C), but > also cleans up some of the code conceptually as well. We can possible > just make RegionBindingsRef -> RegionBindings, but I wanted to stage > things. > > After this, we should revisit Jordan's optimization of not canonicalizing > the immutable AVL trees for the cluster bindings as well. > > Modified: > cfe/trunk/lib/StaticAnalyzer/Core/RegionStore.cpp > > Modified: cfe/trunk/lib/StaticAnalyzer/Core/RegionStore.cpp > URL: > http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/StaticAnalyzer/Core/RegionStore.cpp?rev=169571&r1=169570&r2=169571&view=diff > ============================================================================== > --- cfe/trunk/lib/StaticAnalyzer/Core/RegionStore.cpp (original) > +++ cfe/trunk/lib/StaticAnalyzer/Core/RegionStore.cpp Thu Dec 6 19:55:21 2012 > @@ -132,7 +132,132 @@ > //===----------------------------------------------------------------------===// > > typedef llvm::ImmutableMap<BindingKey, SVal> ClusterBindings; > -typedef llvm::ImmutableMap<const MemRegion *, ClusterBindings> > RegionBindings; > + > +typedef llvm::ImmutableMap<const MemRegion *, ClusterBindings> > + RegionBindings; > + > +namespace { > +class RegionBindingsRef : public llvm::ImmutableMapRef<const MemRegion *, > + ClusterBindings> { > + ClusterBindings::Factory &CBFactory; > +public: > + typedef llvm::ImmutableMapRef<const MemRegion *, ClusterBindings> > + ParentTy; > + > + RegionBindingsRef(ClusterBindings::Factory &CBFactory, > + const RegionBindings::TreeTy *T, > + RegionBindings::TreeTy::Factory *F) > + : ImmutableMapRef(T, F), CBFactory(CBFactory) {} > + > + RegionBindingsRef(const ParentTy &P, ClusterBindings::Factory &CBFactory) > + : ImmutableMapRef(P), CBFactory(CBFactory) {} > + > + RegionBindingsRef add(key_type_ref K, data_type_ref D) { > + return RegionBindingsRef(static_cast<ParentTy*>(this)->add(K, D), > + CBFactory); > + } > + > + RegionBindingsRef remove(key_type_ref K) { > + return RegionBindingsRef(static_cast<ParentTy*>(this)->remove(K), > + CBFactory); > + } > + > + RegionBindingsRef addBinding(BindingKey K, SVal V); > + > + RegionBindingsRef addBinding(const MemRegion *R, > + BindingKey::Kind k, SVal V); > + > + RegionBindingsRef &operator=(const RegionBindingsRef &X) { > + *static_cast<ParentTy*>(this) = X; > + return *this; > + } This is the default copy-constructor; better to leave it out. > //===----------------------------------------------------------------------===// > // Fine-grained control of RegionStoreManager. > @@ -164,19 +289,15 @@ > namespace { > > class RegionStoreManager : public StoreManager { > +public: > const RegionStoreFeatures Features; > RegionBindings::Factory RBFactory; > - ClusterBindings::Factory CBFactory; > + mutable ClusterBindings::Factory CBFactory; Is there a reason this is mutable? When is the RSM ever const? (Left over from an intermediate attempt?) > > // 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. > // 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. > 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. _______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
