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

[Shilei Tian via Phabricator via Openmp-commits]

tianshilei1992 marked 3 inline comments as done.
tianshilei1992 added inline comments.


================
Comment at: openmp/libomptarget/src/device.cpp:31
+      DataMapMtx(), PendingGlobalsMtx(), ShadowMtx(),
+      LoopTripCnt(D.LoopTripCnt), MemoryManager(nullptr) {}
+
----------------
JonChesterfield wrote:
> tianshilei1992 wrote:
> > JonChesterfield wrote:
> > > tianshilei1992 wrote:
> > > > ye-luo wrote:
> > > > > 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.
> > > > `MemoryManager` will be initialized separately later. The only reason we need this is `std::vector<DeviceTy>` requires it. We don't copy or construct those objects afterwards.
> > > std::vector<DeviceTy> should be content with a move constructor. Then the copy constructor can be = delete.
> > `std::mutex` cannot be moved. That is the only reason we have the copy constructor.
> `std::mutex` can't be copied either. If a new default-initialised mutex is OK as the result of the copy, it would be OK as the result of a move too.
That is beyond the scope of this patch. Since it already has a user-defined copy operator, then let it be.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D81054