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