gribozavr2 accepted this revision. gribozavr2 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: > bazuzi wrote: > > 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. > I do think ArrayRef is the right signature here - SetVector is a slightly > messy impl detail. > > This would mean an unfortunate copy for now but that will go away, see > D153485 (which is waiting on the Formula patch to land) @sammccall While I'd normally agree, querySolver() incorporates the true and false literals into the formula, so it is actually a lot more useful than the raw solver object. Until we have a different representation of boolean literals, I think this patch is the better in terms of API design. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D153805/new/ https://reviews.llvm.org/D153805 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
