steakhal added a comment.
I'm still worried about the fact that you assume that there is a correspondence
between `ElementRegions` and InitListExprs.
I cannot see why this assumption holds, since reinterpret casts might introduce
`ElementRegions` which could mess with this assumption.
Aside from that, I'm okay with the general idea of doing this kind of stuff.
================
Comment at: clang/lib/StaticAnalyzer/Core/RegionStore.cpp:1661-1662
+/// \param ER [in] The given (possibly nested) ElementRegion.
+/// \param MR [out] The root MemRegion wrapped inside ElementRegion. It is
+/// guaranteed to be non-null.
+/// NOTE: The result array is in the reverse order of indirection expression:
----------------
If I'm right then this is called the //base// of the region more often than not.
It would be probably a better choice than using `MR`.
================
Comment at: clang/lib/StaticAnalyzer/Core/RegionStore.cpp:1663-1665
+/// NOTE: The result array is in the reverse order of indirection expression:
+/// arr[1][2][3] -> { 3, 2, 1 }. This helps to provide complexity O(n), where n
+/// is a number of indirections.
----------------
IMO this is not a concern in real-life code.
================
Comment at: clang/lib/StaticAnalyzer/Core/RegionStore.cpp:1666-1667
+/// is a number of indirections.
+SmallVector<SVal, 2> getElementRegionOffsets(const ElementRegion *ER,
+ const MemRegion *&MR) {
+ assert(ER && "ConstantArrayType should not be null");
----------------
I would prefer returning a pair instead of an output parameter.
================
Comment at: clang/lib/StaticAnalyzer/Core/RegionStore.cpp:1782-1783
- // Array should be one-dimensional.
- // TODO: Support multidimensional array.
- if (isa<ConstantArrayType>(CAT->getElementType())) // is multidimensional
- return None;
+ // Get a canonical type of the array.
+ CAT = cast<ConstantArrayType>(CAT->getCanonicalTypeInternal());
----------------
I think you can hide this within the `getConstantArrayExtents()`. That is the
only function touching this variable anyway.
That way you could also spare the comments about `CAT` requiring it to be
//canonical//.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D111654/new/
https://reviews.llvm.org/D111654
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
