[PATCH] D127898: [clang][dataflow] Find unsafe locs after fixpoint

[Yitzhak Mandelbaum via Phabricator via cfe-commits]

ymandel added inline comments.


================
Comment at: clang/include/clang/Analysis/FlowSensitive/DataflowAnalysis.h:35
 
+using NoopDiags = std::tuple<>;
+
----------------
I'm not sure that this is valid style. It's essentially a unit type, which I quite like, but I've never seen this done before.


================
Comment at: clang/include/clang/Analysis/FlowSensitive/DataflowAnalysis.h:64
+///
+/// `DiagsT` must have a default constructor.
+template <typename Derived, typename LatticeT, typename DiagsT = NoopDiags>
----------------
Please describe this parameter.


================
Comment at: clang/include/clang/Analysis/FlowSensitive/DataflowAnalysis.h:103
 
+  virtual void check(const Stmt *Stmt, Diags &E, const Lattice &L,
+                     const Environment &Env) {}
----------------
The virtual function seems out of place here, given that everything else is statically resolved. Either drop `virtual` or move to the dyn. typed super class.

That said, I'm not clear on why this isn't a free function. I imagine a model of creating the analysis, running it and that results in blocks annotated with `Environment`s and custome lattices.  Then, you run any function you want over that using some visitor template helper. To account for state, we either make it an explicit parameter or take a `std::function`.


================
Comment at: clang/include/clang/Analysis/FlowSensitive/MatchSwitch.h:52
+  /// Current set of diagnostics.
+  DiagsT &Diags;
+  const Environment &Env;
----------------
this implies that we want to support some form of accumulation. I'd argue that any accumulation should already occur in the lattice (or the environment) and this should be a straight read of the state and some output. So, the `TransferState` struct should be enough.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D127898