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

[Stanislav Gatev via Phabricator via cfe-commits]

sgatev added inline comments.


================
Comment at: clang/include/clang/Analysis/FlowSensitive/Value.h:39
+    // represent conjunctions, disjunctions, and negations.
+    AtomicBoolValue,
+    BoolConjunctionValue,
----------------
xazax.hun wrote:
> 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?
We don't need the `Value` suffix in `Kind` and we don't expect other conjunctions, disjunctions, and negations for now so I simplified the names.


================
Comment at: clang/include/clang/Analysis/FlowSensitive/Value.h:79
+/// Models a boolean conjunction.
+class BoolConjunctionValue : public BoolValue {
+public:
----------------
xazax.hun wrote:
> 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.
Great point! Right now we have this subdivision only for booleans and, as you mentioned, it's not too bad. Definitely worth considering the alternative design going forward. If you don't mind, I suggest starting with the current setup, with a FIXME to remind us of the other option. I'd be happy to revisit this after the next couple of patches.


================
Comment at: clang/lib/Analysis/FlowSensitive/Transfer.cpp:98
+
+      // Create fresh values for unknown boolean expressions.
+      if (LHSVal == nullptr)
----------------
xazax.hun wrote:
> I wonder if this `GetOrCreateFresh` style operations will be common enough to have a helper for this purpose.
That, or we can ensure that each expression gets assigned a value. Added a FIXME for now.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119953