[Openmp-commits] [PATCH] D81054: [OpenMP] Introduce target memory manager

[Ye Luo via Phabricator via Openmp-commits]

ye-luo added inline comments.


================
Comment at: openmp/libomptarget/src/MemoryManager.h:26
+class MemoryManagerTy {
+  std::unique_ptr<impl::MemoryManagerImplTy> Impl;
+
----------------
tianshilei1992 wrote:
> ye-luo wrote:
> > tianshilei1992 wrote:
> > > ye-luo wrote:
> > > > Why is the pointer needed?
> > > > What is the design logic behind MemoryManagerTy and MemoryManagerImplTy layers? Can we just have one?
> > > Pimpl. Like my previous comments mentioned before, this header will be included by others, I don't want unnecessary headers/declarations/definitions to be included to pollute others.
> > That is the job of header and cpp files.
> No. You can refer to https://en.cppreference.com/w/cpp/language/pimpl for more details.
> Pimpl. Like my previous comments mentioned before, this header will be included by others, I don't want unnecessary headers/declarations/definitions to be included to pollute others.

Where else do you have in mind this header will be included? So far there is only device.cpp.


================
Comment at: openmp/libomptarget/src/device.cpp:31
+      DataMapMtx(), PendingGlobalsMtx(), ShadowMtx(),
+      LoopTripCnt(D.LoopTripCnt), MemoryManager(nullptr) {}
+
----------------
Why do you think it is OK here leaving the copy constructor always setting MemoryManager nullptr? This cause surprises. The same question applies to assign operator as well.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D81054/new/

https://reviews.llvm.org/D81054