gribozavr2 added inline comments.
================
Comment at: clang/include/clang/Analysis/FlowSensitive/Diagnosis.h:38-42
+llvm::DenseSet<Diag> diagnoseCFG(
+ const ControlFlowContext &CFCtx,
+ std::vector<llvm::Optional<TypeErasedDataflowAnalysisState>> &BlockStates,
+ const Environment &Env, TypeErasedDataflowAnalysis &Analysis,
+ Diagnosis<Lattice, llvm::DenseSet<Diag>> Diagnose) {
----------------
samestep wrote:
> xazax.hun wrote:
> > xazax.hun wrote:
> > > I feel a bit uneasy about this API. The need of helpers in the tests is a
> > > signal that this API might not be ergonomic as is. One needs to pass a
> > > lot of stuff in, and it is really error prone. For example, the user
> > > might get confused what `Environment` to pass in. Or one might pass the
> > > wrong `Analysis` if there are multiple similar ones in scope. I wonder if
> > > there is a way to reduce the ceremony needed to get diagnostics out. How
> > > about adding an optional argument to `runDataflowAnalysis` function to
> > > collect the results? I wonder if that would be a bit cleaner. You do not
> > > need to change the return type of that function, the collector object
> > > could take a reference to the `llvm::DenseSet<Diag>` that it populates.
> > Moreover, if we pass the diagnostic function to `runDataflowAnalysis`, the
> > fact whether we collect diagnostic during the fixed-point iteration or
> > after that in a separate pass becomes an implementation detail. We can
> > easily change back and forth without affecting the callers. The current API
> > mandates an implementation approach and makes it harder to change in the
> > future.
> I think this makes a lot of sense. I agree that the current set of parameters
> is unwieldy. I hadn't thought about your point of hiding the separate pass as
> an implementation detail, but that also makes sense to me. @gribozavr2
> @ymandel @sgatev thoughts?
I think that's a good idea, it will make a simpler API for typical usage
scenarios. I still think there might be space for an API like diagnoseCFG
(e.g., if you want to run multiple visitations of the analysis results), but we
don't have a use case for it so far.
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
