xazax.hun added inline comments.
================
Comment at: clang/include/clang/Analysis/FlowSensitive/DataflowAnalysis.h:114
bool isEqualTypeErased(const TypeErasedLattice &E1,
const TypeErasedLattice &E2) final {
const Lattice &L1 = llvm::any_cast<const Lattice &>(E1.Value);
----------------
Maybe this is another case where we want to mark the whole class `final`
instead of the individual methods? It is fine to address this in a separate PR.
================
Comment at: clang/include/clang/Analysis/FlowSensitive/DataflowAnalysis.h:135
+ // between overloads.
+ struct Rank1 {};
+ struct Rank0 : Rank1 {};
----------------
Is there ever a reason to instantiate `Rank1`? If no, should we do something
(like make its ctor private and friends with `Rank0`?). Although possibly not
important enough to worth the hassle.
================
Comment at: clang/include/clang/Analysis/FlowSensitive/DataflowAnalysis.h:108
+ LatticeJoinEffect widenTypeErased(TypeErasedLattice &Current,
+ const TypeErasedLattice &Previous) final {
----------------
ymandel wrote:
> xazax.hun wrote:
> > I wonder if `LatticeJoinEffect` is the right name if we also use this for
> > widening. (Also, are we in the process of removing these?)
> I had the same thought. How about just `LatticeEffect`? Open to other
> suggestions as well. Either way, though, I should update in a separate patch
> in case that breaks something unexpected.
>
> As for removing -- yes, we should remove from join, because we never use it.
> But, it actually makes sense here.
`LatticeEffect` sounds good to me.
================
Comment at: clang/include/clang/Analysis/FlowSensitive/DataflowEnvironment.h:465
+ // FIXME: move the fields `CallStack`, `ReturnLoc` and `ThisPointeeLoc` into
a
+ // separate call-context object, shared between environment in the same call.
+ // https://github.com/llvm/llvm-project/issues/59005
----------------
================
Comment at: clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp:424-425
+ }
+ if (WidenedEnv.DeclToLoc.size() != PrevEnv.DeclToLoc.size() ||
+ WidenedEnv.ExprToLoc.size() != PrevEnv.ExprToLoc.size() ||
+ WidenedEnv.LocToVal.size() != PrevEnv.LocToVal.size() ||
----------------
Isn't some of these tautologically false, as we only read from them?
================
Comment at: clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp:374-378
+ Environment WidenedEnv(*DACtx);
+
+ WidenedEnv.CallStack = CallStack;
+ WidenedEnv.ReturnLoc = ReturnLoc;
+ WidenedEnv.ThisPointeeLoc = ThisPointeeLoc;
----------------
ymandel wrote:
> xazax.hun wrote:
> > Shouldn't we have a simple copy ctor from PrefEnv that could populate all
> > these fields?
> >
> > Also, this makes me wonder if we actually want to move some of these out
> > from environment, since these presumably would not change between basic
> > blocks.
> > Shouldn't we have a simple copy ctor from PrefEnv that could populate all
> > these fields?
> We *do* have such a copy constructor. The problem is that it's overkill
> because we want to build LocToVal ourselves by point-wise widening of the
> elements. For that matter, I wish we could share, rather than copy,
> `DeclToLoc` and `ExprToLoc` but I don't know of any way to do that.
> Alternatively: is it possible that we can update in place and don't need a
> separate `WidenedEnv`?
>
> > Also, this makes me wonder if we actually want to move some of these out
> > from environment, since these presumably would not change between basic
> > blocks.
> Agreed. Added a fixme with an associated issue to track.
>
>
> For that matter, I wish we could share, rather than copy, DeclToLoc and
> ExprToLoc.
One way to maximize sharing (and making copies free) is to use the immutable
datastructures in LLVM:
https://github.com/llvm/llvm-project/blob/main/llvm/include/llvm/ADT/ImmutableMap.h
Unfortunately, these have their own costs so it might not be easy to decide
when it is worth to use them. The Clang Static Analyzer was designed from the
ground up with immutable data structures in mind.
On the other hand, I see that you want a more ad-hoc/opportunistic sharing. And
I actually wonder if we need any sharing at all. If we are 100% sure we will do
the widening, do we actually need to preserve the previous environment?
Couldn't we "consume" it and move certain fields out instead of doing the more
expensive copy operation when it makes sense?
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D137948/new/
https://reviews.llvm.org/D137948
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
