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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
