[PATCH] D114234: [clang][dataflow] Add base types for building dataflow analyses

[Stanislav Gatev via Phabricator via cfe-commits]

sgatev added inline comments.


================
Comment at: clang/include/clang/Analysis/FlowSensitive/DataflowAnalysis.h:48
+/// Type-erased base class for dataflow analyses built on a single lattice type.
+class DataflowAnalysisDynamic {
+public:
----------------
xazax.hun wrote:
> Does the `Dynamic` in the suffix refer to the fact this is using type erasure as opposed to templates? 
> 
> I have multiple questions at this point:
> * Why do we prefer type erasure over generic programming?
> * Do you plan to have non-dynamic counterparts?
> 
> Nit: having Dynamic both in the class name and in the method names sounds overly verbose to me.
> Nit: please add a comment what dynamic refers to in the name,
Right. The "Dynamic" suffix emphasizes the type-erased nature of the class and its members and is used to differentiate them from their typed counterparts in `DataflowAnalysis`. I added this to the documentation of the type. I also split the typed and type-erased interfaces in separate files. Users of the framework shouldn't need to interact with types and functions from `DataflowAnalysisDynamic.h`.

The names are verbose, but should be used in relatively few internal places in the framework. Currently, we have one call site for each of the methods of `DataflowAnalysisDynamic`. Nevertheless, if you have ideas for better names I'd be happy to change them.

The reason we went with a type-erased layer is to avoid pulling a lot of code in the templates. Over time the implementation got cleaner and as I'm preparing these patches I see some opportunities to simplify it further. However, it's still non-trivial amount of code. I think we should revisit this decision at some point and consider having entirely template-based implementation of the algorithm. I personally don't see clear benefits of one approach over the other at this point. If you have strong preference for using templates, we can consider going down that route now.


================
Comment at: clang/include/clang/Analysis/FlowSensitive/DataflowAnalysis.h:140
+struct DataflowAnalysisState {
+  DataflowAnalysisState(AnyLatticeElement Lattice, Environment Env)
+      : Lattice(std::move(Lattice)), Env(std::move(Env)) {}
----------------
xazax.hun wrote:
> Do we need a ctor? Or is this a workaround for aggregate initialization not working well with certain library facilities?
We don't really need it at this point. Removed.


================
Comment at: clang/lib/Analysis/FlowSensitive/DataflowAnalysis.cpp:26
+runDataflowAnalysis(const CFG &Cfg, DataflowAnalysisDynamic &Analysis,
+                    const Environment &InitEnv) {
+  // FIXME: Implement work list-based algorithm to compute the fixed
----------------
xazax.hun wrote:
> Is there a way to query how the CFG was built? Adding an assert to see if `setAlwaysAdd` was set might be useful.
Good point. I believe there isn't a way to do that currently, but this is something we should consider implementing. I added a FIXME.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D114234