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

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added inline comments.


================
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,
----------------
xazax.hun wrote:
> 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.
I also believe in "static > dynamic" when it comes to type checks.


================
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);
----------------
xazax.hun wrote:
> 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.
Hmm, should we turn this into a proper variadic function then?


================
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);
----------------
xazax.hun wrote:
> Shouldn't the return value be at least a SubRegion as well?
Yep!


https://reviews.llvm.org/D26838