[PATCH] D128521: [clang][dataflow] Implement functionality to compare if two boolean values are equivalent.

Fri Jun 24 07:05:06 PDT 2022

wyt added inline comments.

================
Comment at: clang/lib/Analysis/FlowSensitive/DataflowAnalysisContext.cpp:138

+bool DataflowAnalysisContext::equivalentBoolValues(BoolValue &Val1,
+                                                   BoolValue &Val2) {
----------------
sgatev wrote:
> This seems unrelated to flow conditions (is this intentional?) and a bit too specific for `DataflowAnalysisContext`. Perhaps we should expose the solver and let user code use it the way it needs to.
> This seems unrelated to flow conditions (is this intentional?) and a bit too specific for `DataflowAnalysisContext`. Perhaps we should expose the solver and let user code use it the way it needs to.

Yes, we noticed this. Unfortunately, creation and using of values is tightly tied to `DataflowAnalysisContext` currently, since the context is responsible for owning the values. @gribozavr2 and I were discussing about refactoring this logic out, some ideas we considered include 
1. A class responsible for communicating with the solver, responsible for building input and processing output from the solver. The context and this class would have a circular dependency. The context relies on this class to interface with the solver, the class relies on the context to create and use values.
2. A further refactoring could be moving the ownership of Values into another object, e.g. a `DataflowArena` maybe. Objects which need Values would have this object as a field.

I chose to stick with this implementation for now and to consider the refactoring in a future patch so that pointer nullability analysis relying on `buildAndSubstituteFlowCondition` will not be blocked by this.

@gribozavr2: Please add if I missed something.
@all: Let me know your thoughts on this, and if you have any ideas as well.
Thanks!!

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128521