ymandel marked 3 inline comments as done and 2 inline comments as done.
ymandel added inline comments.
================
Comment at: clang/include/clang/Analysis/FlowSensitive/DataflowAnalysis.h:68
+/// `LatticeT` can optionally provide the following members:
+/// * `bool widen(const LatticeT &Previous)` - chooses a lattice element that
+/// approximates the join of this element with the argument. Widen is
called
----------------
xazax.hun wrote:
> We should document what is the return value for. Also I see
> `LatticeJoinEffect` instead of bool in the code, but I might be confused.
No, you're right. :) I've completely rewritten the comments -- they were
outdated and wrong. They now match what we have in TypeErasedDatafllowAnalysis.
Most importantly, we've dropped any mention of defaulting to `join` which is
wrong both in fact and conceptually. Our targeted use of widen means it
should not default to `join` -- that would be pointless, since it's
prerequisites would guarantee that `join(prev, cur) = cur`, making the join
pointless. It instead defaults to an equivalence check.
================
Comment at: clang/include/clang/Analysis/FlowSensitive/DataflowAnalysis.h:108
+ LatticeJoinEffect widenTypeErased(TypeErasedLattice &Current,
+ const TypeErasedLattice &Previous) final {
----------------
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.
================
Comment at: clang/include/clang/Analysis/FlowSensitive/DataflowEnvironment.h:227
+ /// Widens the environment point-wise, using `PrevEnv` as needed to inform
the
+ /// approximation. by taking the intersection of storage locations and values
+ /// that are stored in them. Distinct values that are assigned to the same
----------------
li.zhe.hua wrote:
>
I actually just deleted most of the comment -- I don't think the details were
helpful.
================
Comment at: clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp:374-378
+ Environment WidenedEnv(*DACtx);
+
+ WidenedEnv.CallStack = CallStack;
+ WidenedEnv.ReturnLoc = ReturnLoc;
+ WidenedEnv.ThisPointeeLoc = ThisPointeeLoc;
----------------
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.
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
