[PATCH] D153493: [dataflow] avoid more accidental copies of Environment

[Yitzhak Mandelbaum via Phabricator via cfe-commits]

ymandel added a comment.

Just FYI (since you mentioned it in your description): LatticeEffect is vestigial and should be removed. It's now only used (properly) for widening.

Regarding the design.  This looks like an optimal solution in terms of copying but introduces lifetime risks and complexity that I'm uncomfortable with. There's a lot of flexibility here and I wonder if we can reduce that without (substantially?) impacting the amount of copying. Specifically, I wonder if we can have the builder *always* own an environment, which simplifies the handling of `CurrentState` and, generally, the number of cases to consider. It costs more in principle, but maybe negligible in practice?



================
Comment at: clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp:613
             mergeDistinctValues(Loc->getType(), *Val, *this, *It->second, Other,
                                 JoinedEnv, Model)) {
       JoinedEnv.LocToVal.insert({Loc, MergedVal});
----------------
nit: i think you can now drop the braces.


================
Comment at: clang/lib/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.cpp:208
+    if (!CurrentState) {
+      CurrentState = &State;
+    } else if (!OwnedState) {
----------------
Is there some invariant that guarantees the safety here? Because `const &` can bind to a temporary, so this feels like a risky API.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153493