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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
