gribozavr2 added inline comments.
================ Comment at: clang/include/clang/Analysis/FlowSensitive/Diagnosis.h:38 +template <typename Lattice, typename Diag> +llvm::DenseSet<Diag> diagnoseCFG( + const ControlFlowContext &CFCtx, ---------------- This function seems pretty general and not necessarily tied to diagnostics. WDYT about using "visitor" in the name? For example, "visitDataflowAnalysisResults". The "Diagnosis" would also become "DataflowAnalysisResultVisitor". ================ Comment at: clang/include/clang/Analysis/FlowSensitive/Diagnosis.h:42 + const Environment &Env, TypeErasedDataflowAnalysis &Analysis, + Diagnosis<Lattice, llvm::DenseSet<Diag>> Diagnose) { + llvm::DenseSet<Diag> Diags; ---------------- I'm leaning towards std::vector<Diag> instead of a set, to avoid requiring that Diag is hashable. Users who need hash-based deduplication can easily do it themselves (but why would they have duplication? we only visit each Stmt once) ================ Comment at: clang/include/clang/Analysis/FlowSensitive/Diagnosis.h:57 + } + return std::move(Diags); +} ---------------- sgatev wrote: > Better to remove this and rely on NRVO? > https://en.cppreference.com/w/cpp/language/copy_elision https://stackoverflow.com/questions/14856344/when-should-stdmove-be-used-on-a-function-return-value ================ Comment at: clang/include/clang/Analysis/FlowSensitive/MatchSwitch.h:49 +template <typename DiagsT> struct DiagnoseState { + DiagnoseState(DiagsT &Diags, const Environment &Env) + : Diags(Diags), Env(Env) {} ---------------- samestep wrote: > sgatev wrote: > > samestep wrote: > > > sgatev wrote: > > > > Move this to Diagnosis.h? > > > I'd prefer to keep it in `MatchSwitch.h` alongside `TransferState`, > > > unless we plan to, e.g., move that type to `DataflowAnalysis.h`. > > Fair enough, but generally I see `TransferState` and `DiagnoseState` as > > very specific to the dataflow analysis framework whereas > > `MatchSwitchBuilder` seems to be a bit more generic so I'd consider > > separating them. > Sure, if people think it makes sense to put those two types elsewhere then we > can do that in a followup. For now, since there is only a single user, I'd actually prefer to move it into UncheckedOptionalAccessDiagnosis itself. It is a trivial component, basically a std::pair, it seems that exposing it as a public API might not be that helpful. ================ Comment at: clang/include/clang/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.h:78 +public: + UncheckedOptionalAccessDiagnosis( + ASTContext &AstContext, UncheckedOptionalAccessModelOptions Options = {}); ---------------- "Diagnosis" sounds like the result. Should this be a "Diagnoser"? ================ Comment at: clang/unittests/Analysis/FlowSensitive/TestingSupport.h:92 std::function<AnalysisT(ASTContext &, Environment &)> MakeAnalysis, - std::function<void( - llvm::ArrayRef<std::pair< - std::string, DataflowAnalysisState<typename AnalysisT::Lattice>>>, - ASTContext &)> - Expectations, + std::function<void(AnalysisResults)> Expectations, ArrayRef<std::string> Args, ---------------- It is better to name a function with a verb. WDYT about "VerifyResults"? If you agree, please also update the functions below. ================ Comment at: clang/unittests/Analysis/FlowSensitive/TestingSupport.h:148 + llvm::StringRef Code, + ast_matchers::internal::Matcher<FunctionDecl> FuncMatcher, + std::function<AnalysisT(ASTContext &, Environment &)> MakeAnalysis, ---------------- ================ Comment at: clang/unittests/Analysis/FlowSensitive/TestingSupport.h:190 + }); + } + Expectations(Results, AnalysisResults.Context); ---------------- Can we use diagnoseCFG to implement the loop above? ================ Comment at: clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp:1925 // )", - // UnorderedElementsAre(Pair("check-1", "unsafe: input.cc:10:9"), - // Pair("check-2", "safe"))); + // "safe"); } ---------------- The original was unsafe, is it an intentional change? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D127898/new/ https://reviews.llvm.org/D127898 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits