[PATCH] D153469: [dataflow] Replace most BoolValue subclasses with references to Formula (and AtomicBoolValue => Atom and BoolValue => Formula where appropriate)

[Sam McCall via Phabricator via cfe-commits]

sammccall marked 3 inline comments as done.
sammccall added inline comments.


================
Comment at: clang/include/clang/Analysis/FlowSensitive/Arena.h:61
+  /// Passing in the same formula will result in the same BoolValue.
+  /// FIXME: This canonicalization isn't sound e.g. if we create two BoolValues
+  ///        with the same formula but attach different properties.
----------------
gribozavr2 wrote:
> Per our offline conversation, do you still believe in this fixme?
No, I'm no longer convinced "unsound" is true.

However, I do think there's something to be fixed: if we really believe in it, we should be interning other types of values too (or have a clear idea about why bools are special).

And I think I still lean towards removing it: it looks like a power-vs-complexity tradeoff in the Value concept, and I suspect the historical reason to intern values was to intern formulas, not because we needed the power.

Reworded the fixme to say we should decide and be more consistent - text doesn't take a position on which way we should decide.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153469