xazax.hun added a comment. In general I really like this change. See some of my comments inline.
================ Comment at: include/clang/StaticAnalyzer/Core/PathSensitive/MemRegion.h:1279 /// associated element type, index, and super region. const ElementRegion *getElementRegion(QualType elementType, NonLoc Idx, + const SubRegion *superRegion, ---------------- a.sidorin wrote: > I think we should perform a `cast<>` to `SubRegion` internally in order to > keep API simple and do not force clients to introduce casts in their code. > This will still allow us to keep nice suggestions about SubRegions/MemSpaces > in our hierarchy. I prefer having a stricter static type. ================ Comment at: include/clang/StaticAnalyzer/Core/PathSensitive/MemRegion.h:1347 private: - template <typename RegionTy, typename A1> - RegionTy* getSubRegion(const A1 a1, const MemRegion* superRegion); + template <typename RegionTy, typename SuperTy, typename A1> + RegionTy* getSubRegion(const A1 a1, const SuperTy* superRegion); ---------------- a.sidorin wrote: > Maybe we should give types and paramers some more meaningful names as a part > of refactoring? At least, the number in `A1` is not needed now. Agreed. ================ Comment at: include/clang/StaticAnalyzer/Core/PathSensitive/Store.h:238 protected: - const MemRegion *MakeElementRegion(const MemRegion *baseRegion, + const MemRegion *MakeElementRegion(const SubRegion *baseRegion, QualType pointeeTy, uint64_t index = 0); ---------------- Shouldn't the return value be at least a SubRegion as well? ================ Comment at: lib/StaticAnalyzer/Core/ExprEngineCXX.cpp:492 + const SubRegion *NewReg = + cast<SubRegion>(symVal.castAs<loc::MemRegionVal>().getRegion()); QualType ObjTy = CNE->getType()->getAs<PointerType>()->getPointeeType(); ---------------- Maybe using getRegionAs would be shorter? ================ Comment at: lib/StaticAnalyzer/Core/Store.cpp:439 + const SubRegion *BaseRegion = + cast<SubRegion>(Base.castAs<loc::MemRegionVal>().getRegion()); ---------------- Also consider getRegionAs. https://reviews.llvm.org/D26838 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits