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

[Balázs Kéri via Phabricator via cfe-commits]

balazske added inline comments.


================
Comment at: clang/include/clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h:403
   /// frame. May fail; returns null on failure.
-  const VarRegion *getParameterLocation(unsigned Index,
-                                        unsigned BlockCount) const;
+  const TypedValueRegion *getParameterLocation(unsigned Index,
+                                               unsigned BlockCount) const;
----------------
I would expect that this returns a `ParamRegion` (from the name of the function). (The implementation shows that the type can just be changed.)


================
Comment at: clang/lib/StaticAnalyzer/Checkers/ValistChecker.cpp:176-180
   if (const auto *DeclReg = Reg->getAs<DeclRegion>()) {
     if (isa<ParmVarDecl>(DeclReg->getDecl()))
       Reg = C.getState()->getSVal(SV.castAs<Loc>()).getAsRegion();
+  } else if (const auto *ParamReg = Reg->getAs<ParamRegion>()) {
+    Reg = C.getState()->getSVal(SV.castAs<Loc>()).getAsRegion();
----------------
baloghadamsoftware wrote:
> Szelethus wrote:
> > This is interesting. I looked up `DeclRegion`, and it seems to be the region that is tied to a `ValueDecl`. `VarDecl` is a subtype of `ValueDecl`, and `ParmVarDecl` is a subtype of `VarDecl`, so wouldn't it make sense for `ParamRegion` to be a subtype of `VarRegion`?
> `DeclRegion` stores the `Decl`, `ParamRegion` retrieves it based on the `Index` it stores. There is no is-a relation between them.
During the lifetime of a `ParamRegion` is it possible that it will return different `Decl` objects?


================
Comment at: clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp:745
 
     // Can only reasonably pretty-print DeclRegions.
+    if (const auto *DR = dyn_cast<DeclRegion>(R)) {
----------------
This comment is not fully true any more.


================
Comment at: clang/lib/StaticAnalyzer/Core/CallEvent.cpp:255
   // correspond to the stack frame's function declaration.
   assert(VR->getStackFrame() == SFC);
 
----------------
Is it correct to create `VR` only to have this assert?


================
Comment at: clang/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp:357
           V = *OptV;
-        else
-          break;
+        else break;
         State = addObjectUnderConstruction(State, {CE, Idx}, LCtx, V);
----------------
This `break` is at wrong place.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D79704/new/

https://reviews.llvm.org/D79704