[PATCH] D137948: [clang][dataflow] Add widening API and implement it for built-in boolean model.

[Gábor Horváth via Phabricator via cfe-commits]

xazax.hun 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
----------------
We should document what is the return value for. Also I see `LatticeJoinEffect` instead of bool in the code, but I might be confused.


================
Comment at: clang/include/clang/Analysis/FlowSensitive/DataflowAnalysis.h:108
 
+  LatticeJoinEffect widenTypeErased(TypeErasedLattice &Current,
+                                    const TypeErasedLattice &Previous) final {
----------------
I wonder if `LatticeJoinEffect` is the right name if we also use this for widening. (Also, are we in the process of removing these?)


================
Comment at: clang/include/clang/Analysis/FlowSensitive/DataflowEnvironment.h:140
+    ///
+    ///  `Prev` must precede `Current` in the value ordering.
+    ///
----------------
First, I was confused, because I thought they could be equivalent as well. But I guess this code is only called for values that are not considered equivalent. Documenting this is really useful.


================
Comment at: clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp:374-378
+  Environment WidenedEnv(*DACtx);
+
+  WidenedEnv.CallStack = CallStack;
+  WidenedEnv.ReturnLoc = ReturnLoc;
+  WidenedEnv.ThisPointeeLoc = ThisPointeeLoc;
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137948