[PATCH] D144977: [analyzer] Fix of the initialization list parsing.

[Eänolituri Lómitaurë via Phabricator via cfe-commits]

earnol requested review of this revision.
earnol added inline comments.


================
Comment at: clang/lib/StaticAnalyzer/Core/RegionStore.cpp:1787
+  if (V &&
+      (!targetType->isStructureOrClassType() && !targetType->isUnionType()))
     return *V;
----------------
isuckatcs wrote:
> I assume `targetType` is the type we want to interpret the region as. Below this condition we seem to work with arrays. If `targetType` is an array, then we return something here instead of going further and returning something else we probably want. 
> 
> Why aren't we going further in that case?
A good comment and the answer would be because i wanted to limit my changes to the case of the struct/array of structs/nested structs only. To properly implement support of all cases the changes should be more global and more intrusive. My goal was to limit a scope and make baby step improvement.
In short: because this code is not written yet and adding what you suggested will increase scope more than i wanted.


================
Comment at: clang/lib/StaticAnalyzer/Core/RegionStore.cpp:1869
+      // if we are here we have struct or union?
+      if (!VarT->isStructureType()) {
+        // TODO: support other options like unions or arrays or VLAs
----------------
isuckatcs wrote:
> What about classes? 
> 
> ```
> class A { 
> public:
>   int x;
> };
> 
> struct B {
>   int x;
> }
> ```
> 
> `A` and `B` are technically the same, but `A` will fall into the true branch, `B` will fall into the false branch.
Classes are currently not supported and were not tested. But adding class support would be no brainier here. However i wanted to limit the scope of the changes.


================
Comment at: clang/lib/StaticAnalyzer/Core/RegionStore.cpp:1891
+        ElemExpr = IL->getInit(Idx);
+        std::optional<SVal> ConstVal = svalBuilder.getConstantVal(ElemExpr);
+        // if there is no value create a zero one
----------------
isuckatcs wrote:
> This crashes if `ElemExpr` is a `nullptr`.
Accepted. `nullptr` should never pop up with correctly constructed initialization list in the line 1890, but nobody promised me to have a correct initialization list here.


================
Comment at: clang/lib/StaticAnalyzer/Core/RegionStore.cpp:1914
+          }
+          RecIter++;
+          continue;
----------------
isuckatcs wrote:
> Consider moving this into the for loop to avoid confusion.
If i am to do it i'll need to move line 1883 into the for loop as well for consistency making for loop bulky and cumbersome. Do you really think it will make code easier to understand?
I propose to move iterator advance somewhere line 1890 so all `for` associated operation will be close to the for operator. How about it?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D144977/new/

https://reviews.llvm.org/D144977