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

Thu Jun 23 09:24:22 PDT 2022

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

================
Comment at: clang/include/clang/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.h:85
+private:
+  ASTContext &Context;
+  MatchSwitch<DiagnoseState<llvm::DenseSet<SourceLocation>>>
----------------
Do we expect this to modify the ASTContext? Could this be a const ref instead?

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D127898