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
