[PATCH] D129495: [Orc][JITLink] JITLinkMemoryManager implementation using MemoryMapper

[Lang Hames via Phabricator via llvm-commits]

lhames added a comment.

Otherwise LGTM. Nice work!



================
Comment at: llvm/include/llvm/ExecutionEngine/Orc/MemoryMapper.h:46
+  // Page size of the target process
+  virtual unsigned int getPageSize() = 0;
+
----------------
`SharedMemoryMapper` will need a definition of this now too.


================
Comment at: llvm/lib/ExecutionEngine/Orc/MemoryMapper.cpp:20
+  if (!PageSize)
+    PageSize = cantFail(sys::Process::getPageSize());
+}
----------------
`getPageSize` can fail -- this needs to return an error. The usual idiom is a named constructor:


  class InProcessMemoryMapper ... {
  public:
    Expected<InProcessMemoryMapper> Create() {
      if (auto PageSize = sys::Process::getPageSize())
        return InProcessMemoryMapper(*PageSize);
      else
        return PageSize.takeError();
    }

    InProcessMemoryMapper(size_t PageSize) : PageSize(PageSize) {}

    ...
  }


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D129495