[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 cfe-commits at lists.llvm.org
Wed Nov 23 05:44:53 PST 2016

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
-  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.

Comment at: include/clang/StaticAnalyzer/Core/PathSensitive/Store.h:238
-  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.


More information about the cfe-commits mailing list