jdoerfert marked an inline comment as done. jdoerfert added inline comments.
================ Comment at: llvm/include/llvm/Frontend/OpenMP/OMPIRBuilder.h:51 + /// at the time, and location, the callback is invoked. + using FinalizeCallbackTy = std::function<void(InsertPointTy /* CodeGenIP */)>; + ---------------- ABataev wrote: > jdoerfert wrote: > > ABataev wrote: > > > jdoerfert wrote: > > > > ABataev wrote: > > > > > jdoerfert wrote: > > > > > > ABataev wrote: > > > > > > > `llvm::function_ref`? > > > > > > The lambda that is passed in here might go out of scope so we need > > > > > > to own it. This is intentional. > > > > > Maybe better to save the intermediate data in CGOpenMPRuntime class > > > > > rather than rely on owning lambda ref here? Clang does not use > > > > > escaping decls and instead stores intermediate data explicitly. It > > > > > really complicates analysis and potential source of resource leakage. > > > > I don't follow. Clang does use `std::function` to store callbacks that > > > > have to life for a while. Why is this different? What would be the > > > > benefit of having a function_ref here and a `std::function` in > > > > CGOpenMPRuntime? Note that the FinaliztionInfo object is created in the > > > > front-end and the std::function is assigned there already. > > > Clang uses it only in some rare cases, when it is really required, like > > > typo correction or something like this. In other cases it is not > > > recommended to use it. > > > > > > I'm not saying that you need to store `std::function` in CGOpenMPRuntime > > > class, I'm saying about necessary data. > > What necessary data? Can you please explain how you want it to look like > > and why? This version seems to work fine. > I'm not arguing that it does not work, I'm asking do you really need such a > complex solution? Just like I said before, it is very hard to maintain and to > understand the lifetime of lambda, so it is a potential source of resource > leakage. All I'm asking is `is there a way to implement it differently`, > nothing else. There are alternatives, all of which are more complex and come with the same "problems". Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D70258/new/ https://reviews.llvm.org/D70258 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
