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

[Sam McCall via Phabricator via cfe-commits]

sammccall marked an inline comment as done.
sammccall added a comment.

> Regarding the design. This looks like an optimal solution in terms of copying but introduces lifetime risks and complexity that I'm uncomfortable with.

FWIW I agree with the complexity (not the lifetime risks).
I think it ultimately stems from having too many interacting types & functions with complicated and inconsistent signatures & constraints (TypeErasedDataflowAnalysisState, Environment, TypeErasedLattice, Lattice, the various join functions, etc) so we resort to case-bashing.
However this system is difficult to make changes to, and getting consensus as to what changes are good also doesn't seem easy. So I think we need to be able to fix bugs within the existing design (where copies need to be avoided ad-hoc).

> 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?

I don't understand the specific suggestion:

- if we create the builder lazily, we're just moving handling this extra state into the callsites
- if the builder owns a copy of initenv, its FC token will end up in the result FC
- if the builder owns a copy of initenv and detects when it should discard it, that just sounds like nullptr with extra steps

can you clarify?



================
Comment at: clang/lib/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.cpp:208
+    if (!CurrentState) {
+      CurrentState = &State;
+    } else if (!OwnedState) {
----------------
ymandel wrote:
> Is there some invariant that guarantees the safety here? Because `const &` can bind to a temporary, so this feels like a risky API.
The invariant is that an external/unowned environment will outlive the joiner, which is just a temporary helper object.

Added comments to the class & CurrentState.

> const & can bind to a temporary

given that the method name is **addUnowned** I think "i shouldn't pass null" is more important to communicate than "I shouldn't pass a temporary", though.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153493