martong added a comment.
Thanks for addressing my comments!
================
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;
----------------
baloghadamsoftware wrote:
> martong wrote:
> > Is this used anywhere in this patch?
> >
> > And why isn't it a `CallExpr` (instead of `Expr`)?
> The origin expression of a call may be a set of different kinds of
> expressions: `CallExpr`, `CXXConstructExpr`, `BlockExpr` or `ObjCMessageExpr`.
Is this function used anywhere in this patch? Could not find any reference to
it.
> The origin expression of a call may be a set of different kinds of
> expressions: CallExpr, CXXConstructExpr, BlockExpr or ObjCMessageExpr.
Okay, makes sense, could you please document it then in the comment for the
function? And perhaps we should assert on these kinds as a sanity check.
================
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)
----------------
baloghadamsoftware wrote:
> martong wrote:
> > 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`.
> Usually we do not inherit from concrete classes by changing a method
> implementation that already exist. The other reason is even stronger:
> `NonParamVarRegion` //must// store the `Decl` of the variable,
> `ParamVarRegion` //must not//.
> Usually we do not inherit from concrete classes by changing a method
> implementation that already exist.
That's what we exactly do all over the place in the AST library.
Which method(s) should we change by the way?
Nevermind, I am perfectly fine having VarRegion as base and NonParamVarRegion
and ParamVarRegion as derived.
================
Comment at: clang/lib/StaticAnalyzer/Core/MemRegion.cpp:1230
+
+const TypedValueRegion*
+MemRegionManager::getRegionForParam(const ParmVarDecl *PVD,
----------------
baloghadamsoftware wrote:
> martong wrote:
> > Why the return type is not `ParamVarRegion*`?
> Because for top-level functions and captured parameters it still returns
> `VarRegion`.
Okay, makes sense, could you please document it then in the comment for the
function?
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D79704/new/
https://reviews.llvm.org/D79704
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
