[PATCH] D120984: [clang][dataflow] Extend flow conditions from block terminators

[Stanislav Gatev via Phabricator via cfe-commits]

sgatev added inline comments.


================
Comment at: clang/include/clang/Analysis/FlowSensitive/DataflowEnvironment.h:99
+                       const Environment &Env2, Value &MergedVal,
+                       Environment &Env) {
       return false;
----------------
xazax.hun wrote:
> `MergedEnv`? Also, the documentation above gives a short description of the relationship between `Val1`, `Val2`, and `MergedVal`. But it gives little pointers what are we supposed to do with the `Environment`. 
Updated the name and added some pointers in the documentation.


================
Comment at: clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp:154
+
+  // `X v (X ^ Y ^ ...)` is logically equivalent to `X`. The common conditions
+  // have already been added to the result so we don't have to do anything here
----------------
ymandel wrote:
> The current comment gets at the big picture, but focusing on the actual disjunction that is being guarded (that is, just the remaining, unshared clauses) may be better. So, maybe instead point out that "True v (X ^ ...)"  is equivalent to "True", since if either val is nullptr it represents "true"?
I added a general comment at the top and modified the comment here to target the specific case.


================
Comment at: clang/lib/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.cpp:112
+
+    // The condition must be inversed in one of the successors.
+    if (BlockSuccIdx == 1)
----------------
ymandel wrote:
> Can we be more specific? I'd think we need to invert for specifically the successor corresponding to "else" or what not.
Yup. Updated the comment to be more specific.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D120984