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