[PATCH] D127898: [clang][dataflow] Add API to separate analysis from diagnosis

[Dmitri Gribenko via Phabricator via llvm-commits]

gribozavr2 added inline comments.


================
Comment at: clang/include/clang/Analysis/FlowSensitive/Diagnosis.h:38-42
+llvm::DenseSet<Diag> diagnoseCFG(
+    const ControlFlowContext &CFCtx,
+    std::vector<llvm::Optional<TypeErasedDataflowAnalysisState>> &BlockStates,
+    const Environment &Env, TypeErasedDataflowAnalysis &Analysis,
+    Diagnosis<Lattice, llvm::DenseSet<Diag>> Diagnose) {
----------------
samestep wrote:
> xazax.hun wrote:
> > xazax.hun wrote:
> > > I feel a bit uneasy about this API. The need of helpers in the tests is a signal that this API might not be ergonomic as is. One needs to pass a lot of stuff in, and it is really error prone. For example, the user might get confused what `Environment` to pass in. Or one might pass the wrong `Analysis` if there are multiple similar ones in scope. I wonder if there is a way to reduce the ceremony needed to get diagnostics out. How about adding an optional argument to `runDataflowAnalysis` function to collect the results? I wonder if that would be a bit cleaner. You do not need to change the return type of that function, the collector object could take a reference to the `llvm::DenseSet<Diag>` that it populates. 
> > Moreover, if we pass the diagnostic function to `runDataflowAnalysis`, the fact whether we collect diagnostic during the fixed-point iteration or after that in a separate pass becomes an implementation detail. We can easily change back and forth without affecting the callers. The current API mandates an implementation approach and makes it harder to change in the future.
> I think this makes a lot of sense. I agree that the current set of parameters is unwieldy. I hadn't thought about your point of hiding the separate pass as an implementation detail, but that also makes sense to me. @gribozavr2 @ymandel @sgatev thoughts?
I think that's a good idea, it will make a simpler API for typical usage scenarios. I still think there might be space for an API like diagnoseCFG (e.g., if you want to run multiple visitations of the analysis results), but we don't have a use case for it so far.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D127898