[PATCH] D26838: [analyzer] Enforce super-region classes for various memory regions through compile-time and run-time type checks.

[Gábor Horváth via cfe-commits]

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