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: > ymandel wrote: > > 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. > > > > > Weird. I think we should change the test to pass the decl. I'd like that, but when looking through other uses found a bunch that pass nullptr (and for no apparent reason - they all have the function decl handy). So, I propose that we mark the new API as such and then in the cleanup we'll make sure to pass a non-null argument. ================ Comment at: clang/lib/Analysis/FlowSensitive/DataflowAnalysisContext.cpp:19 #include "clang/Analysis/FlowSensitive/Value.h" +#include "clang/Basic/IdentifierTable.h" #include "llvm/Support/Debug.h" ---------------- sgatev wrote: > Is this still needed? Nope. 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
