[clang] [analyzer] Limit Store by region-store-binding-limit (PR #127602)

[Donát Nagy via cfe-commits]

================
@@ -176,31 +177,59 @@ class RegionBindingsRef : public llvm::ImmutableMapRef<const MemRegion *,
   // however that would have made the manager needlessly stateful.
   bool IsMainAnalysis;
 
+  unsigned BindingsLeft;
+
 public:
+  unsigned bindingsLeft() const { return BindingsLeft; }
+
+  bool hasExhaustedBindingLimit() const { return BindingsLeft == 0; }
+
+  RegionBindingsRef escapeValue(SVal V) const {
+    assert(EscapedValuesDuringBind);
+    EscapedValuesDuringBind->push_back(V);
+    return *this;
+  }
+  RegionBindingsRef escapeValues(nonloc::CompoundVal::iterator Begin,
+                                 nonloc::CompoundVal::iterator End) const {
+    for (SVal V : llvm::make_range(Begin, End))
+      escapeValue(V);
+    return *this;
+  }
+
   typedef llvm::ImmutableMapRef<const MemRegion *, ClusterBindings>
           ParentTy;
 
   RegionBindingsRef(ClusterBindings::Factory &CBFactory,
+                    SmallVectorImpl<SVal> *EscapedValuesDuringBind,
                     const RegionBindings::TreeTy *T,
-                    RegionBindings::TreeTy::Factory *F,
-                    bool IsMainAnalysis)
-      : llvm::ImmutableMapRef<const MemRegion *, ClusterBindings>(T, F),
-        CBFactory(&CBFactory), IsMainAnalysis(IsMainAnalysis) {}
-
-  RegionBindingsRef(const ParentTy &P,
-                    ClusterBindings::Factory &CBFactory,
-                    bool IsMainAnalysis)
-      : llvm::ImmutableMapRef<const MemRegion *, ClusterBindings>(P),
-        CBFactory(&CBFactory), IsMainAnalysis(IsMainAnalysis) {}
+                    RegionBindings::TreeTy::Factory *F, bool IsMainAnalysis,
+                    unsigned BindingsLeft)
+      : RegionBindingsRef(ParentTy(T, F), CBFactory, EscapedValuesDuringBind,
+                          IsMainAnalysis, BindingsLeft) {}
+
+  RegionBindingsRef(const ParentTy &P, ClusterBindings::Factory &CBFactory,
+                    SmallVectorImpl<SVal> *EscapedValuesDuringBind,
+                    bool IsMainAnalysis, unsigned BindingsLeft)
+      : ParentTy(P), CBFactory(&CBFactory),
+        EscapedValuesDuringBind(EscapedValuesDuringBind),
+        IsMainAnalysis(IsMainAnalysis), BindingsLeft(BindingsLeft) {}
+
+  RegionBindingsRef add(key_type_ref K, data_type_ref D,
----------------
NagyDonat wrote:

It's very confusing that this class has two methods called `add` and `addBinding`. I initially thought that they do essentially the same thing (with some extra higher-level bookkeeping in `addBinding`), but after reading them at least four times I finally realized that `add` is adding a cluster and not a single binding. (This issue was heavily exacerbate by the fact that the real types were hidden behind the meaningless aliases `key_type_ref` and `data_type_ref`.)

Please rename `add` to e.g. `addCluster` to highlight this difference and prevent similar confusion for anyone who reads this code. This would have the added benefit that this way you wouldn't need to use the ugly `static_cast<const ParentTy *>(this)->add` syntax to refer to the `add` method which was inherited from the `ParentTy` aka `llvm::ImmutableMapRef<const MemRegion *, ClusterBindings>`. (Btw I don't like the idea that this class is publicly derived from the region→cluster map instead of just owning it as a data member, but that's another story.)

(By the way, I know that this is not a new issue within this code, but as you're heavily refactoring this code, it would be easy to fix it.)

https://github.com/llvm/llvm-project/pull/127602