ABataev 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 */)>; + ---------------- 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. 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
