[PATCH] D63453: [Support] Move llvm::MemoryBuffer to sys::fs::file_t

[Reid Kleckner via Phabricator via llvm-commits]

rnk added inline comments.


================
Comment at: llvm/include/llvm/Support/FileSystem.h:963
+/// no-op.
+file_t convertFDToNativeFile(int FD);
+
----------------
aganea wrote:
> rnk wrote:
> > aganea wrote:
> > > Why not use `file_t` everywhere? Do we still need POSIX FD's in user code? I find it error-prone having to juggle between the two.
> > No reason, other than that there's a lot of code to migrate. The refactoring is also made slightly more difficult by the move to use `llvm::Error` instead of `std::error_code`.
> Do you think that's something that should be done? (not having FDs anymore)
Yes.


================
Comment at: llvm/lib/Support/Windows/Path.inc:1211
+
+std::error_code readNativeFile(file_t FileHandle, MutableArrayRef<char> Buf,
+                               size_t *BytesRead) {
----------------
aganea wrote:
> rnk wrote:
> > aganea wrote:
> > > Just keep the API below that "works" all the time? (`readNativeFileSlice`)
> > Are you suggesting simplifying the code by forwarding the call with offset 0, or reducing the API to one entry point?
> > 
> > Stream devices like pipes don't support seeking or pread, so to change the public API, I would need to make the offset optional, which I think would be less clear. Perhaps I can simplify this Windows specific code to delegate to the slice version, but that was my concern.
> I meant forward to a common implementation for both `readNativeFile` and `readNativeFileSlice`
Sure, done.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D63453