[PATCH] D144977: [analyzer] Fix of the initialization list parsing.
Eänolituri Lómitaurë via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Thu Mar 2 14:09:09 PST 2023
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
More information about the cfe-commits
mailing list