[PATCH] D80522: [Analyzer] [NFC] Parameter Regions -- Alternative Approach

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added a comment.

Nice, looks like you managed to reuse most of the code. I still feel like we should ditch `DeclRegion` entirely (forcing its ~5 current users consider its specific variants separately) but i don't insist.



================
Comment at: clang/include/clang/StaticAnalyzer/Checkers/SValExplainer.h:232-233
+    OS << Index;
+    if (Index % 10 == 1 && Index != 11)
+      OS << "st ";
+    else if (Index % 10 == 2 && Index != 12)
----------------
There's a standard function for that. I never remember what it's called, something something numeric something plural something suffix, but it's there.


================
Comment at: clang/lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp:539-540
       (void)VV;
-      assert(cast<VarRegion>(VV.castAs<loc::MemRegionVal>().getRegion())
-                 ->getStackFrame()->getParent()
-                 ->getStackFrame() == LC->getStackFrame());
+      if (const auto *VR =
+          dyn_cast<VarRegion>(VV.castAs<loc::MemRegionVal>().getRegion())) {
+        assert(VR->getStackFrame()->getParent()
----------------
Why did you relax this `cast<>` to `dyn_cast<>`?


================
Comment at: clang/lib/StaticAnalyzer/Core/MemRegion.cpp:183
+  assert (getDecl() &&
+          "`ParamVarRegion` does not support functions without `Decl`");
+  return getDecl()->getType();
----------------
It will eventually have to. Maybe `"...not implemented yet"?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80522