[PATCH] D153674: [dataflow] Disallow implicit copy of Environment, use fork() instead

[Sam McCall via Phabricator via cfe-commits]

sammccall added inline comments.


================
Comment at: clang/include/clang/Analysis/FlowSensitive/DataflowAnalysis.h:211
       PostVisitCFG(Element, DataflowAnalysisState<typename AnalysisT::Lattice>{
-                                *Lattice, State.Env});
+                                *Lattice, State.Env.fork()});
     };
----------------
these copies shouldn't be happening (or if we're really going to do them, PostVisitCFG should get the env by value!)

However I think fixing it means changing the signature of PostVisitCFG...


================
Comment at: clang/include/clang/Analysis/FlowSensitive/DataflowAnalysis.h:228
+        return llvm::transformOptional(
+            std::move(OptState), [](TypeErasedDataflowAnalysisState &&State) {
+              return DataflowAnalysisState<typename AnalysisT::Lattice>{
----------------
with `auto&&` this fails to compile, complaining about the copy constructor being deleted.

Not sure exactly what's going on - maybe the wrong overload of transformOptional is called for some reason.

In any case, I'm fairly sure we were accidentally copying here.


================
Comment at: clang/lib/Analysis/FlowSensitive/Transfer.cpp:858
 
-    auto ExitState = (*BlockToOutputState)[ExitBlock];
+    auto &ExitState = (*BlockToOutputState)[ExitBlock];
     assert(ExitState);
----------------
this looks like it was an accidental copy


================
Comment at: clang/unittests/Analysis/FlowSensitive/TestingSupport.h:366
         auto [_, InsertSuccess] = AnnotationStates.insert(
-            {It->second, StateT{State.Lattice, State.Env}});
+            {It->second, StateT{State.Lattice, State.Env.fork()}});
         (void)_;
----------------
this seems legitimate to me: the only way to preserve the Environment from halfway through a BB is to fork it


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153674