martong added a comment. Seems like many changes could be simplified or absolutely not needed in this patch if ParamRegion (or with a better name ParamVarRegion) was derived from VarRegion. Is there any difficulties in that derivation?
================ Comment at: clang/include/clang/StaticAnalyzer/Core/PathSensitive/ProgramState.h:309 + /// Get the lvalue for a parameter. + Loc getLValue(const Expr *Call, unsigned Index, + const LocationContext *LC) const; ---------------- Is this used anywhere in this patch? And why isn't it a `CallExpr` (instead of `Expr`)? ================ Comment at: clang/lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountChecker.cpp:519 + return false; // CleanupAttr attaches destructors, which cause escaping. + } else { + const ParmVarDecl *PVD = PR->getDecl(); ---------------- Again, ParamRegion should derive from VarRegion. ================ Comment at: clang/lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountDiagnostics.cpp:439 + VD = cast<VarDecl>(VR->getDecl()); + } else if (const auto *PR = + dyn_cast<ParamRegion>(cast<SymbolRegionValue>(Sym)->getRegion())) { ---------------- This would be simpler (noop?) again if ParamRegion was derived from VarRegion. ================ Comment at: clang/lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountDiagnostics.cpp:618 return std::string(VR->getDecl()->getName()); + if (const auto *PR = dyn_cast_or_null<ParamRegion>(MR)) + return std::string(PR->getDecl()->getName()); ---------------- What if ParamRegion was derived from VarRegion ? ================ Comment at: clang/lib/StaticAnalyzer/Core/MemRegion.cpp:195 + if (Index >= FD->param_size()) + return nullptr; + ---------------- In all other cases we `assert`, here we return with a nullptr. Why? ================ Comment at: clang/lib/StaticAnalyzer/Core/MemRegion.cpp:915 + return cast<VarRegion>(I.getCapturedRegion()); + } else if (const auto *PR = dyn_cast<ParamRegion>(OrigR)) { + if (PR->getDecl() == VD) ---------------- Both the `VarRegion` and `ParamRegion` cases here do the same. This suggest that maybe it would be better to have `VarRegion` as a base class for `ParamVarRegion`. This is aligned to what Artem suggested: > We can even call it VarRegion and have sub-classes be ParamVarRegion and > NonParamVarRegion or something like that. But maybe `NonParamVarRegion` is not needed since that is actually the `VarRegion`. ================ Comment at: clang/lib/StaticAnalyzer/Core/MemRegion.cpp:1230 + +const TypedValueRegion* +MemRegionManager::getRegionForParam(const ParmVarDecl *PVD, ---------------- Why the return type is not `ParamVarRegion*`? ================ Comment at: clang/lib/StaticAnalyzer/Core/SymbolManager.cpp:580 + +bool SymbolReaper::isLive(const ParamRegion *PR, + bool includeStoreBindings) const{ ---------------- This is almost the copy-paste code for isLive(VarRegion). This again suggests that maybe ParamRegion (or ParamVarRegion?) should be derived from VarRegion. And in isLive(VarRegion) we should dyn_cast to ParamVarRegion and do the extra stuff if needed. 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