[cfe-commits] r110020 - in /cfe/trunk: include/clang/Checker/PathSensitive/MemRegion.h lib/Checker/FlatStore.cpp lib/Checker/MemRegion.cpp lib/Checker/RegionStore.cpp lib/Checker/SimpleSValuator.cpp lib/Checker/Store.cpp test/Analysis/flat-store.c

[Ted Kremenek]

On Aug 1, 2010, at 9:56 PM, Zhongxing Xu <xuzhongxing at gmail.com> wrote:

> +  /// Compute the offset within the top level memory object.
> +  virtual RegionOffset getAsOffset() const {
> +    assert(0 && "unimplemented");
> +  }

Instead of embedding the algorithm for computing RegionOffsets inside methods of SubRegion and it's derived classes, perhaps we should separate them into their own functions (or a factory class).  After all, not all clients of MemRegion care about offsets, and to keep our APIs clean in LLVM we tend to not like to embed algorithms that operate on datatypes into the datatypes themselves.  I'm also not a fan of using assertions in this way to catch unimplemented functionality, as it results in crashes (assertion failures) for users.  If we put this logic in one function, for example, we could have a switch statement off the MemRegion kind and the compiler would tell us when we missed something. 

What also bothers me is that the notion of an offset seems somewhat tailored to the expectations of a given StoreManager.  I'm concerned that the needs of FlatStoreManager and RegionStoreManager may diverge at some point, and may wish to compute offsets differently.  That wouldn't be ideal to diverge that logic, but it is something we should keep in mind when develop this API.