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

[Sam Estep via Phabricator via cfe-commits]

samestep marked 9 inline comments as done.
samestep added a comment.

Fixed all the minor nits and responded to some comments.



================
Comment at: clang/lib/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.cpp:401
+                           const TypeErasedDataflowAnalysisState &State) {
+            PostVisitStmt(Stmt.getStmt(), State);
+          });
----------------
sgatev wrote:
> Let's harmonize the arguments to `transferBlock` and `runTypeErasedDataflowAnalysis`. I think in both cases we only need access to the internal `Stmt` so there's no need to pass `CFGStmt` to `transferBlock`. This way we can pass `PostVisitStmt` directly here without having to wrap it in a lambda.
Makes sense to me; should I do that in a followup?


================
Comment at: clang/unittests/Analysis/FlowSensitive/TestingSupport.h:87-89
+    std::function<std::function<void(
+        const Stmt *, const TypeErasedDataflowAnalysisState &)>(ASTContext &)>
+        MakePostVisitStmt,
----------------
sgatev wrote:
> Why is this a function that returns a function? Couldn't it be simply a void function, like `VerifyResults`?
It needs to take in the `ASTContext`, and it needs to be able to close over state; see the one we pass from `UncheckedOptionalAccessModelTest.cpp`.


================
Comment at: clang/unittests/Analysis/FlowSensitive/TestingSupport.h:172-191
+        std::vector<std::pair<std::string, StateT>> Results;
+        for (const CFGBlock *Block : AnalysisResults.CFCtx.getCFG()) {
+          // Skip blocks that were not evaluated.
+          if (!AnalysisResults.BlockStates[Block->getBlockID()])
+            continue;
+
+          transferBlock(
----------------
ymandel wrote:
> any chance that this could be folded into `MakePostVisitStmt`?
Looking into this now.


================
Comment at: clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp:2338
   //       if (opt1.has_value()) {
   //         opt2.value();
   //       }
----------------
gribozavr2 wrote:
> 
I don't think this is correct, is it?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D127898