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

[Gabor Marton via Phabricator via cfe-commits]

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