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

[Alexandre Ganea via Phabricator via llvm-commits]

aganea added inline comments.


================
Comment at: llvm/include/llvm/Support/FileSystem.h:938
 ///          platform-specific error_code.
 std::error_code openFile(const Twine &Name, int &ResultFD,
                          CreationDisposition Disp, FileAccess Access,
----------------
Same question as below, why not just keep `openNativeFile`?


================
Comment at: llvm/include/llvm/Support/FileSystem.h:963
+/// no-op.
+file_t convertFDToNativeFile(int FD);
+
----------------
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.


================
Comment at: llvm/lib/Support/Windows/Path.inc:738
+std::error_code status(file_t FileHandle, file_status &Result) {
+  return getStatus(FileHandle, Result);
+}
----------------
A bit unrelated, but worth mentioning: I've noticed that `status()` (and thus `getStatus()`) came up in profiles as slower-than-they-should-be. Each call to `status()` issues 5 kernel calls and opens the file by the same occasion. In comparaison, MSVC uses the file metadata through the `FindFirstFile/FindNextFile` API, which is a lot faster. When using precompiled headers, this makes a difference because Clang calls `status()` on every file referenced by the PCH, whereas MSVC simply iterates through folders (for a given CPP using precompiled headers, MSVC was taking ~250 ms to compile, and Clang about ~1200 ms). Clang has some level of caching, but even at that it goes through the `status()` call. One of the ideas was maybe to hide a directory caching mechanism behind this `status()` API instead of letting it go directly to the OS layer.


================
Comment at: llvm/lib/Support/Windows/Path.inc:1211
+
+std::error_code readNativeFile(file_t FileHandle, MutableArrayRef<char> Buf,
+                               size_t *BytesRead) {
----------------
Just keep the API below that "works" all the time? (`readNativeFileSlice`)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D63453