samestep added inline comments.
================ Comment at: clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp:220 + assert(Body != nullptr); + initGlobalVars(*Body, Env); + ---------------- ymandel wrote: > samestep wrote: > > ymandel wrote: > > > samestep wrote: > > > > ymandel wrote: > > > > > I wonder how this will work between caller and callee. Do we need > > > > > separate global var state in the frame? If so, maybe mention that as > > > > > well in the FIXME above. > > > > Could you clarify what you mean? Perhaps I just don't understand > > > > exactly what is meant by "global vars" here. > > > https://github.com/llvm/llvm-project/blob/main/clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp#L131-L135 > > > > > > ``` > > > /// Initializes global storage values that are declared or referenced from > > > /// sub-statements of `S`. > > > // FIXME: Add support for resetting globals after function calls to enable > > > // the implementation of sound analyses. > > > ``` > > > Since this already mentions a need to reset after function calls, seemed > > > relevant here. > > Hmm, OK. I pretty much just pattern-matched from the `Environment` > > constructor right above this method implementation. Would it be better for > > me to instead just remove this `initGlobalVars` call for now and replace it > > with a `FIXME` saying that we'll probably need to call this but it's > > unclear how exactly to do so? > SGTM. We can note this constraint on models' reference implementations. > Specifically, that they cannot reference globals. Done. ================ Comment at: clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp:231 + // parameters still need to be given `StorageLocation`s anyway, so this code + // will need to be generalized later. + for (; ArgIt != ArgEnd; ++ParamIt, ++ArgIt) { ---------------- gribozavr2 wrote: > The Clang AST includes argument expressions for defaulted arguments, so I > believe there shouldn't be anything left to do here, it should just work. Oh nice! I'm updating this comment, thanks. ================ Comment at: clang/unittests/Analysis/FlowSensitive/TransferTest.cpp:3896 + *cast<BoolValue>(Env.getValue(*FooDecl, SkipPast::None)); + EXPECT_FALSE(Env.flowConditionImplies(FooVal)); + }, ---------------- gribozavr2 wrote: > Good idea, done. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D130306/new/ https://reviews.llvm.org/D130306 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
