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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
