[PATCH] D153805: Expose DataflowAnalysisContext.querySolver().

[Samira Bazuzi via Phabricator via cfe-commits]

bazuzi added inline comments.


================
Comment at: clang/include/clang/Analysis/FlowSensitive/DataflowAnalysisContext.h:183
+  /// been stored in flow conditions.
+  Solver::Result querySolver(llvm::DenseSet<BoolValue *> Constraints);
+
----------------
sammccall wrote:
> sammccall wrote:
> > FWIW, I'd probably prefer exposing the solver object itself, having all capabilities exposed directly through DataflowAnalysisContext gives it this ugly "god object" quality and the places that we want to use it really just need arena + solver.
> this should be ArrayRef<BoolValue*> now... sorry for the churn
HEAD says SetVector, so updated to that. Is there another change coming that makes it ArrayRef?


================
Comment at: clang/include/clang/Analysis/FlowSensitive/DataflowAnalysisContext.h:183
+  /// been stored in flow conditions.
+  Solver::Result querySolver(llvm::DenseSet<BoolValue *> Constraints);
+
----------------
bazuzi wrote:
> sammccall wrote:
> > sammccall wrote:
> > > FWIW, I'd probably prefer exposing the solver object itself, having all capabilities exposed directly through DataflowAnalysisContext gives it this ugly "god object" quality and the places that we want to use it really just need arena + solver.
> > this should be ArrayRef<BoolValue*> now... sorry for the churn
> HEAD says SetVector, so updated to that. Is there another change coming that makes it ArrayRef?
If all capabilities was more than 1 capability and this function didn't already exist, I'd me more inclined to agree. But requiring users to duplicate the body of this function seems worse to me than forwarding an API from a member.

I'm noticing as well that the body of this function adds true and !false constraints to the provided set, which I hadn't been doing when using a solver and for which I can find no documentation indicating that it's necessary. Either we should document why that's done and would need to expect users to know to do it if we expose the solver only directly, or we should remove that from this function.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153805