samestep added inline comments.
================ Comment at: clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp:208 + + // TODO: Currently this only works if the callee is never a method and the + // same callee is never analyzed from multiple separate callsites. To ---------------- ymandel wrote: > OK, I'll change this; would you like for me to replace all the other `TODO`s with `FIXME`s, as well? ================ Comment at: clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp:220 + assert(Body != nullptr); + initGlobalVars(*Body, Env); + ---------------- 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. ================ Comment at: clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp:237 + const Expr *Arg = *ArgIt; + // TODO: Confirm that this is the correct `SkipPast` to use here. + auto *ArgLoc = Env.getStorageLocation(*Arg, SkipPast::None); ---------------- ymandel wrote: > I'm pretty sure we want `SkipPast::Reference`. That will ensure that the > parameter and argument share the same underlying location. Otherwise, in the > case of references, the parameter will point to the reference location object > rather than just directly to the location. OK, thank you! I'll make that change. ================ Comment at: clang/lib/Analysis/FlowSensitive/Transfer.cpp:517 + + // TODO: Cache these CFGs. + auto CFCtx = ControlFlowContext::build(F, F->getBody(), &ASTCtx); ---------------- ymandel wrote: > here and below: s/TODO/FIXME. Will do. ================ Comment at: clang/lib/Analysis/FlowSensitive/Transfer.cpp:524 + auto CalleeEnv = Env.pushCall(S); + auto Analysis = NoopAnalysis(ASTCtx, /*ApplyBuiltinTransfer=*/true); + ---------------- ymandel wrote: > This seems worth a FIXME or, at least, an explanation. It implies that with > the current design, we can't support general-purpose analyses, which we > should probably fix. Given our goal of supporting models that don't involve > specialized lattices, I think this is a good compromise for the short term, > but not a stable solution for the framework (hence FIXME sounds right). Good point, and @xazax.hun pointed this out as well. I'll add a `FIXME` here, at least. 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
