[PATCH] D111654: [analyzer] Retrieve a value from list initialization of multi-dimensional array declaration.

[Balázs Benics via Phabricator via cfe-commits]

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