[PATCH] D79704: [Analyzer] [NFC] Parameter Regions

[Gabor Marton via Phabricator via cfe-commits]

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