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

[Pavel Labath via Phabricator via llvm-commits]

labath added inline comments.


================
Comment at: llvm/trunk/lib/Support/Windows/Path.inc:1258-1262
+    // Once we reach EOF, zero the remaining bytes in the buffer.
+    if (BytesRead == 0) {
+      memset(BufPtr, 0, BytesLeft);
+      break;
+    }
----------------
aganea wrote:
> labath wrote:
> > BTW, I've was looking at this code in context of D66344, and I have found this quirk of this API to be very surprising and unexpected. Zeroing the remainder of the buffer may be the right behavior for `MemoryBuffer::getFileSlice` (though I'm not even convinced of that), but I definitely wouldn't expect that to happen in `readNativeFileSlice`. I think that an additional `size_t *BytesRead` read argument would be more natural here (and consistent with `readNativeFile`).
> > 
> > What would you say to a patch which adds a BytesRead argument to this function and moves the zeroing code to the MemoryBuffer class (currently the only user of this function)?
> Sounds good to me! `ErrorOr<size_t> readNativeFileSlice(..)` ? (and same thing for `readNativeFile` ?)
Cool. I've created D66471 for that. I've went for `Expected<size_t>` instead of `ErrorOr`, as (IIUC) the plan is to eventually get rid of `ErrorOr`..


Repository:
  rL LLVM

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

https://reviews.llvm.org/D63453