[PATCH] D119953: [clang][dataflow] Add transfer functions for logical and, or, not.

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

xazax.hun added inline comments.


================
Comment at: clang/include/clang/Analysis/FlowSensitive/Transfer.h:28
+
+  /// Returns the environment of the basic block that contains `S` or nullptr if
+  /// there isn't one.
----------------
Depending on how willing sensitive we are to memory vs runtime, if there is one environment per basic block, we could look up the basic block of the statement, and use the basic block's id, to look up the environment in a vector.


================
Comment at: clang/include/clang/Analysis/FlowSensitive/Value.h:39
+    // represent conjunctions, disjunctions, and negations.
+    AtomicBoolValue,
+    BoolConjunctionValue,
----------------
Do we need `Value` in the `Kind` if we are already in the class `Value`?
Also, do we need prefix `Conjunction` and `Disjunction` with `Bool`? Will there be a `Conjunction` with other types?


================
Comment at: clang/include/clang/Analysis/FlowSensitive/Value.h:79
+/// Models a boolean conjunction.
+class BoolConjunctionValue : public BoolValue {
+public:
----------------
Do we want to have a separate class for every binary/unary operator? Alternatively, we could follow what the AST is already doing. Having BinaryOperator and UnaryOperator classes with a kind. I guess the current approach is not too bad for booleans, but if we want to support all integer operators as well in the future I wonder if that would end up with too much boilerplate.


================
Comment at: clang/lib/Analysis/FlowSensitive/Transfer.cpp:98
+
+      // Create fresh values for unknown boolean expressions.
+      if (LHSVal == nullptr)
----------------
I wonder if this `GetOrCreateFresh` style operations will be common enough to have a helper for this purpose.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119953