ymandel marked 4 inline comments as done. ymandel added inline comments.
================ Comment at: clang/include/clang/Analysis/FlowSensitive/ControlFlowContext.h:34 + /// Builds a ControlFlowContext from an AST node. `D` is the function in which + /// `S` resides. All arguments must be non-null. static llvm::Expected<ControlFlowContext> build(const Decl *D, Stmt *S, ---------------- sgatev wrote: > Can we make them references? Turns out I was wrong - `D` can be null. Would that CFG has comments about what its parameters do... We have a test that excercises this (though I can't say why): https://github.com/llvm/llvm-project/blob/9a976f36615dbe15e76c12b22f711b2e597a8e51/clang/unittests/Analysis/FlowSensitive/TypeErasedDataflowAnalysisTest.cpp#L70 The other two -- yes -- but then we'd have to update the various callers as well. I'd rather not do that in this patch, but I'll add the new overload and we can follow up with cleanups in another patch. ================ Comment at: clang/include/clang/Analysis/FlowSensitive/ControlFlowContext.h:50 private: - ControlFlowContext(std::unique_ptr<CFG> Cfg, + // `D` must not be null. + ControlFlowContext(const Decl *D, std::unique_ptr<CFG> Cfg, ---------------- sgatev wrote: > Can we make it a reference? See above. ================ Comment at: clang/include/clang/Analysis/FlowSensitive/DataflowAnalysis.h:23 #include "clang/AST/Stmt.h" -#include "clang/Analysis/CFG.h" #include "clang/Analysis/FlowSensitive/ControlFlowContext.h" ---------------- samestep wrote: > I'm guessing this change is unrelated to the patch as a whole? correct. it was a driveby fix (for better or worse). ================ Comment at: clang/include/clang/Analysis/FlowSensitive/DataflowAnalysisContext.h:369 + + // Keyed on the function's fully qualified name. No leading "::". + llvm::StringMap<ControlFlowContext> FunctionModels; ---------------- sgatev wrote: > Can we make the keys `FunctionDecl *` and use `FunctionDecl::getDefinition` > in `getControlFlowContext` to obtain a canonical pointer? We can for now -- for the purposes of the cache that makes the most sense. I thought of strings because the goal is a map from *specific* (named) functions, for which we won't a priori have the function decls. still, I'll change for now and we can change it back (or to a better implementation) in the follow up patches. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D131039/new/ https://reviews.llvm.org/D131039 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
