On Tue, Aug 3, 2010 at 12:21 AM, Ted Kremenek <[email protected]> wrote: > On Aug 1, 2010, at 9:56 PM, Zhongxing Xu <[email protected]> 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.
Actually this is my original implementation. I'll fix this in a later patch. > > 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. > Yes. Currently region store and flat store do different offset computing. _______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
