baloghadamsoftware marked 2 inline comments as done. baloghadamsoftware added a comment.
Thank you for quickly looking into this. In D79704#2029230 <https://reviews.llvm.org/D79704#2029230>, @Szelethus wrote: > - What identifies a `MemRegion`, `TypedValueRegion`s in particular? Why are > parameters so special that they deserve their own region? Do you have a > concrete, problematic example? `ParamRegion` contains different data: mainly the index of the parameter. This makes it independent of the actual `Decl` of the parameter. > - Why is it an issue that we don't always use the same declaration? Does this > result in a crash, incorrect modeling? See D49443#1193290 <https://reviews.llvm.org/D49443#1193290>. > - Why are we making `ParamRegion` **not** a subclass of `VarRegion`? Because `VarRegion` is a `DeclRegion` which stores the `Decl` of the variable. Here the main purpose is to not to store it. > - The patch includes some code duplication, which may be okay, but do we need > it? Yes, and this comes from my previous answers. ================ Comment at: clang/lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountDiagnostics.cpp:435-442 + const VarDecl *VD; + if (const auto *VR = + dyn_cast<VarRegion>(cast<SymbolRegionValue>(Sym)->getRegion())) { + VD = cast<VarDecl>(VR->getDecl()); + } else if (const auto *PR = + dyn_cast<ParamRegion>(cast<SymbolRegionValue>(Sym)->getRegion())) { + VD = cast<ParmVarDecl>(PR->getDecl()); ---------------- Szelethus wrote: > Hmm. So, the surrounding code definitely suggests that we're definitely > finishing for a parameter here, but it isn't always a parameter region? I > know you struggled with this, have you gained any insight as to why? Not all regions for `ParmVarDecl` are `ParamRegions`. Exceptions are parameters captured by a lambda or a block as well as parameters of functions analyzed top-level. ================ Comment at: clang/lib/StaticAnalyzer/Checkers/ValistChecker.cpp:176-180 if (const auto *DeclReg = Reg->getAs<DeclRegion>()) { if (isa<ParmVarDecl>(DeclReg->getDecl())) Reg = C.getState()->getSVal(SV.castAs<Loc>()).getAsRegion(); + } else if (const auto *ParamReg = Reg->getAs<ParamRegion>()) { + Reg = C.getState()->getSVal(SV.castAs<Loc>()).getAsRegion(); ---------------- Szelethus wrote: > This is interesting. I looked up `DeclRegion`, and it seems to be the region > that is tied to a `ValueDecl`. `VarDecl` is a subtype of `ValueDecl`, and > `ParmVarDecl` is a subtype of `VarDecl`, so wouldn't it make sense for > `ParamRegion` to be a subtype of `VarRegion`? `DeclRegion` stores the `Decl`, `ParamRegion` retrieves it based on the `Index` it stores. There is no is-a relation between them. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D79704/new/ https://reviews.llvm.org/D79704 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
