[PATCH] D156254: [clang][dataflow] Add convenience function for analysing and `FunctionDecl` and diagnosing it.

[Martin Böhme via Phabricator via cfe-commits]

mboehme accepted this revision.
mboehme added inline comments.
This revision is now accepted and ready to land.


================
Comment at: clang/include/clang/Analysis/FlowSensitive/DataflowAnalysis.h:244
 
+/// Runs a dataflow analysis over the given function and then runs `Diagnoser`
+/// over the results. If the analysis is successful, returns a list of
----------------
Can you document what the function signature for this should be?

Also, I'm wondering if `DiagnoserT` even needs to be a template argument. Shouldn't the `Diagnostic` type be the only "variable" in the type of `DiagnoserT`?

Or is it that you don't want to force the caller to pass a `std::function` for `Diagnoser`? In that case, you could consider [`function_ref'](https://llvm.org/docs/ProgrammersManual.html#the-function-ref-class-template).




================
Comment at: clang/include/clang/Analysis/FlowSensitive/DataflowAnalysis.h:245-246
+/// Runs a dataflow analysis over the given function and then runs `Diagnoser`
+/// over the results. If the analysis is successful, returns a list of
+/// diagnostics for `FuncDecl`. Otherwise, returns an error. Currently, errors
+/// can occur (at least) because the analysis requires too many iterations over
----------------



================
Comment at: clang/include/clang/Analysis/FlowSensitive/DataflowAnalysis.h:259
+diagnoseFunction(const FunctionDecl &FuncDecl, ASTContext &ASTCtx,
+                 DiagnoserT &Diagnoser,
+                 std::int64_t MaxSATIterations = 1'000'000'000) {
----------------
Does this need to be passed by non-const reference?

`std::function::operator()` is const, so a const reference should be fine for that. Non-const reference looks like we might be assigning to the reference (which we aren't).


================
Comment at: clang/include/clang/Analysis/FlowSensitive/DataflowAnalysis.h:261-265
+  using ::llvm::Expected;
+
+  Expected<ControlFlowContext> Context = ControlFlowContext::build(FuncDecl);
+  if (!Context)
+    return Context.takeError();
----------------



================
Comment at: clang/include/clang/Analysis/FlowSensitive/DataflowAnalysis.h:267-268
+
+  auto Solver = std::make_unique<WatchedLiteralsSolver>(MaxSATIterations);
+  const WatchedLiteralsSolver *SolverView = Solver.get();
+  DataflowAnalysisContext AnalysisContext(std::move(Solver));
----------------
Nit: Of the two variables `Solver` and `SolverView`, `Solver` sounds like the more canonical name and the one to "go for". Granted, this is a short function with little potential for confusion, but maybe call the unique pointer `OwnedSolver` (or similar) and the raw pointer `Solver`?

This would also eliminate the "view", which to me sounds like there's more going on than just a simple pointer.


================
Comment at: clang/include/clang/Analysis/FlowSensitive/DataflowAnalysis.h:273-288
+  auto BlockToOutputState = runTypeErasedDataflowAnalysis(
+      *Context, Analysis, Env,
+      [&ASTCtx, &Diagnoser,
+       &Diagnostics](const CFGElement &Elt,
+                     const TypeErasedDataflowAnalysisState &State) mutable {
+        auto EltDiagnostics =
+            Diagnoser(Elt, ASTCtx,
----------------



================
Comment at: clang/unittests/Analysis/FlowSensitive/TypeErasedDataflowAnalysisTest.cpp:95-98
+  auto *Func = selectFirst<FunctionDecl>(
+      "func", match(functionDecl(ast_matchers::hasName("target")).bind("func"),
+                    AST->getASTContext()));
+  ASSERT_THAT(Func, NotNull());
----------------



================
Comment at: clang/unittests/Analysis/FlowSensitive/TypeErasedDataflowAnalysisTest.cpp:101-103
+  auto Diagnoser = [&Count](const CFGElement &, ASTContext &,
+                            const TransferStateForDiagnostics<NoopLattice> &) {
+    return std::vector<int>({++Count});
----------------
I think this gives us a stronger and more explicit check for little extra effort.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D156254