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

[Gábor Horváth via Phabricator via cfe-commits]

xazax.hun 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) {
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D127898